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

ERC-820 Pseudo-introspection using a registry contract. #906

Merged
merged 9 commits into from
Apr 6, 2018

Conversation

jbaylina
Copy link
Contributor

@jbaylina jbaylina commented Feb 27, 2018

This is the PR discussed in #820
Currently, there is no open issues/conflicts in this standard proposal.

@fulldecent
Copy link
Contributor

fulldecent commented Feb 27, 2018

Notes:

  • Please update ERC references to be links
  • 0xffffffff is not a valid ERC-165 interface, please check for that
  • For ERC820_ACCEPT_MAGIC please consider the value bytes4(keccak256("onNFTReceived(address,uint256,bytes)")), justification is at EIP-721 Non-Fungible Token Standard #841 and https://gitter.im/ETH-NFT/Lobby, I think we can be consistent on this mechanism
  • "You can see the string a at the end." please document this better since this is a new concept
  • Remove periods from end of headings
  • Use backticks: getInterfaceImplementer()
  • Typo: ERC777TokensReceicver
  • Typo: 28bytes
  • Typo: EI165

I can contribute a PR if you consider these issues valid.

@jbaylina
Copy link
Contributor Author

jbaylina commented Feb 28, 2018

@fulldecent , just updated the EIP-820. With all the typos, links, and a better explanation of Nick's deployment method.

Regarding the other 2 issues, I have 2 questions:
1.- ERC820_ACCEPT_MAGIC you propose to use one that contains the word NFT (Non fungible token) for the calculation. I don't really see the point and the logic for what you are proposing. In any case, this is just a nonsense random number that warranties that a function is implemented specifically for that intention.
2.- 0xFFFFFFFF interface it will always return invalid, because it is checked here: https://github.com/jbaylina/eip820/blob/master/contracts/ERC820Registry.sol#L115 So I don't think it's any need to add any extra check.
The remaining comments are fixed!.

@fulldecent
Copy link
Contributor

fulldecent commented Feb 28, 2018

For magic, perhaps you can use:

bytes4(keccak256("canImplementInterfaceForAddress(address,bytes32)"))

It is an arbitrary choice. But someone reviewing our ERC-721 said that we should reuse ERC-165 style naming because we already have a way to turn a function into a bytes4.


Second item 2 Looks good to me!

@fulldecent
Copy link
Contributor

Last item: EIP-X format is to use email addresses in the byline at the top.

@jbaylina
Copy link
Contributor Author

jbaylina commented Mar 1, 2018

@fulldecent In the current implementation, the Magic number is a full word (bytes32) and not a bytes4. The porpoise of this magic number has nothing to do with EIP165. It makes no sense for me to follow that pattern. Making it so similar may bring to confusion.

@fulldecent
Copy link
Contributor

No worries. Thanks for the discussion.

EIPS/eip-820.md Outdated

### The smart contract

```
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 use Solidity syntax highlighting here by using

```solidity

@Arachnid
Copy link
Contributor

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

@Arachnid
Copy link
Contributor

Arachnid commented Apr 6, 2018

Please advise if you'd like this merged as a draft in its current state.

@Arachnid Arachnid merged commit ea5b7b3 into ethereum:master Apr 6, 2018
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request May 2, 2018
* ERC-820 Pseudo-introspection using a registry contract.

* Typos, links, and better explanation of Nicks deployment method

* Formating fix

* Email address

* solidity syntax highlight

* Adapt to new template for EIPs

* Discussions link added

* Fix link in discussion-to

* Type fixed
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.

4 participants