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

EIP-5269: ERC Interface Detection #5269

Merged
merged 22 commits into from
Aug 17, 2022
Merged

EIP-5269: ERC Interface Detection #5269

merged 22 commits into from
Aug 17, 2022

Conversation

xinbenlv
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@eth-bot
Copy link
Collaborator

eth-bot commented Jul 15, 2022

A critical exception has occurred:
Message: pr 5269 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

EIPS/eip-5269.md Outdated
eip: 5269
title: Human Readable Interface Detection
description: An interface returning Human Readable Interface
author: Zainan Victor Zhou (xinbenlv@)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
author: Zainan Victor Zhou (xinbenlv@)
author: Zainan Victor Zhou (@xinbenlv)

EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated
Comment on lines 28 to 29
## Backwards Compatibility
TODO
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Backwards Compatibility
TODO
## Backwards Compatibility
No backward compatibility issues were found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed backward compatibility section, if that's ok

Copy link
Member

Choose a reason for hiding this comment

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

No, it is not. All EIPs must have a backwards compatibility section, even if it is as simple as the one above.

Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference: I usually allow TODO for sections like Backward Compatibility and Security Considerations for Draft EIPs. These sections need to be filled in before Review, but it can be a helpful reminder to fill them with TODO initially so it is more obvious they aren't complete yet.

EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
@xinbenlv xinbenlv changed the title Create eip-5269 Create draft EIP-5269 (ERC): Human Readable Interface Detection Jul 16, 2022
@xinbenlv
Copy link
Contributor Author

The EIPW complains

Error: proposals must be referenced with the form `EIP-N` (not `ERC-N`)

Hi @SamWilsn and @Pandapip1 , I like to ask for an one-time exception: this ERC is about returning ERC numbers and only supports ERC, hence we would love to use ERC to refer to an EIP in this particular case to emphasize such.

@Pandapip1
Copy link
Member

Pandapip1 commented Jul 16, 2022

Hi @SamWilsn and @Pandapip1 , I like to ask for an one-time exception: this ERC is about returning ERC numbers and only supports ERC, hence we would love to use ERC to refer to an EIP in this particular case to emphasize such.

Suggestion: if you replace every instance of ERC (save the one in the preamble) with EIP, this EIP will still make sense and not need an exception.

@MicahZoltu
Copy link
Contributor

The requirement to use EIP-X is for when you are linking to/referencing an EIP in the english text of the document. You can use ERC in your code snippets and non-prose text sections of your specification if you want.

EIPS/eip-5269.md Outdated Show resolved Hide resolved
EIPS/eip-5269.md Outdated Show resolved Hide resolved
@xinbenlv xinbenlv changed the title Create draft EIP-5269 (ERC): Human Readable Interface Detection Create draft EIP-5269: ERC Interface Detection Jul 16, 2022
DELJUJAZI
DELJUJAZI previously approved these changes Jul 17, 2022
@Pandapip1
Copy link
Member

What's the advantage of this over EIP-165 (except for human readability?)

Copy link
Contributor Author

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

Done

@xinbenlv xinbenlv changed the title Create draft EIP-5269: ERC Interface Detection EIP-5269: ERC Interface Detection Aug 14, 2022
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 14, 2022

Unless I'm missing something, this proposal is fundamentally incomplete. E.g. EIP-721 as one of many documents contains multiple EIP-165 identifers e.g. the metadata extension or the enumerable extension. If we were to detect features of an EIP-721 contract, it's not clear how this proposal could fundamentally achieve this.

You are right. The sub/extension of interface is being worked on.

However, there is also something that 165 can't be used for. For example the ERC712 or EIP-54537 smart endorsement #5453 can't be easily represented as the "function signature" in which 165 relies on.

@xinbenlv
Copy link
Contributor Author

@SamWilsn could I request the merge?

@Pandapip1
Copy link
Member

I recommend looking at EIP-1820. It has some good ideas you could borrow.

@xinbenlv
Copy link
Contributor Author

I recommend looking at EIP-1820. It has some good ideas you could borrow.

Thank you. EIP-5269 focuses on contract self-reporting/declaring their interface.

I am aware of EIP-1820 and have got some ideas of how 5269 could work with 1820 or the like (e.g. registering interface somewhere.) but that's probably going to be in its separate scope hence worth a separate new EIP.

@Pandapip1
Copy link
Member

You are right. The sub/extension of interface is being worked on.

I think that this needs to be completed before this can be merged. Currently, the EIP is unimplementable for that reason.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 16, 2022

@Pandapip1 thanks for review. Could you help me find out which particular rule in EIP-1 blocking this snapshot to be merged? I think the "draft" status is meant for WIP drafts?

And also it is a implementable version already: implementer can ignore extension detection. For example, an implementor can declare that its interface implements ERC721 by returning true to 721. Other information is not allowed to be exposed at the current snapshot. Just like EIP-165 doesn't allow OpenZappellin TransparentProxy contract to exposed that they behaves differently based on who the caller is

@Pandapip1
Copy link
Member

Could you help me find out which particular rule in EIP-1 blocking this snapshot to be merged?

There isn't any particular rule, but a fair minimum is that the EIP has to be self-consistent. For e.g. EIP-721, it is not.

@github-actions
Copy link

The commit cde8e36 (as a parent of 3c5ecbe) contains errors. Please inspect the Run Summary for details.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 16, 2022

Added a minor number given a lot of feedback was question about not being able to support ERC optional extension interface. I've no sure if this is optimal yet.

Hopefully by merging it we can get more feedback from draft readers and implementers

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.

I guess if you really want this merged, you can get this merged.

@eth-bot eth-bot enabled auto-merge (squash) August 17, 2022 00:33
@eth-bot eth-bot merged commit 3757a34 into ethereum:master Aug 17, 2022
@xinbenlv
Copy link
Contributor Author

Thanks @Pandapip1 , hoping when this is checked in, it can be used by other EIPs. My sense is when put in practice it better challenge our thinking and help us improve it with more concrete example.

nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Create eip-5269

* Working in progress

* Rename eip-5269 to eip-5269.md

* Update eip-5269.md

* Update EIPS/eip-5269.md

Co-authored-by: Pandapip1 <[email protected]>

* Update EIPS/eip-5269.md

Co-authored-by: Pandapip1 <[email protected]>

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update eip-5269.md

* Update EIPS/eip-5269.md

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-5269.md

Co-authored-by: Sam Wilson <[email protected]>

* Update EIPS/eip-5269.md

Co-authored-by: Sam Wilson <[email protected]>

* Update content

* Simplify and add minor extension.

* Update content

* Update content

Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants