ERC820: Move To Final

[0xjac]

The review end date has passed without further changes, comments or suggestion. Hence I would like to move 820 to a final status.

I'd also like to thank everyone who contributed and helped with ERC820, (including in no particular order): @fulldecent @mcdee @jaycenhorton @recmo @hyperfekt @wighawag @thegostep @Arachnid @Souptacular and everyone I forget.

N.B. @Arachnid @Souptacular could one of you merge this pull request please? Thanks.

[eip-automerger]

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

• Trying to change EIP 820 state from Draft to Final
• EIP 820 requires approval from one of (jacques@dafflon.tech, @jbaylina)

[fulldecent]

Please make a list of all changes effected and all issues discussed during the last call process and the resolution of each. Please ping relevant people that raised issues.

Example: #1170

[0xjac]

@fulldecent sure thing:

Changes

• Immaterial change to the CC0 header of the registry
• Change canImplementInterfaceForAddress(address,bytes32) for canImplementInterfaceForAddress(bytes32,address) (swap of arguments)
• Marked function interfaceHash(string) as external (instead of public)
• No automatic attempt to update the ERC165 cache in the view function implementsERC165Interface(address,byte4).
• Add @dev note to the noThrowCall(address,bytes4).
• Clarify the meaning of the 820 pattern in the signature of the deployment transaction.
• Clarify the meaning of passing a zero-value as the first parameter of setInterfaceImplementer(address,bytes32,address) and getInterfaceImplementer(address,bytes32).
• Remove the default value of msg.sender for the first parameter of setManager(address,address) when a zero-value is passed.
• Various small immaterial fixes such as typo, change of case, etc.

Discussions

• @fulldecent mentioned:

  • The implementation has several locations where an address of zero has the meaning of "the sender". -- RESOLUTION: Clarification of the meaning where an address of zero has the meaning of "the sender" and removal of this meaning in the setOwner(address,address).
  • the argument ordering of canImplementInterfaceForAddress(address addr, bytes32 interfaceHash) is confusing. -- RESOLUTION: Swap the arguments to reduce confusion.
  • he was part of the effort to design the ERC165 cache which was not acknowledged. -- RESOLUTION: His contribution is now acknowledged both in the ERC820 Registry source code and in the EIP.
  • function noThrowCall(address _contract, bytes4 _interfaceId) is missing a dev note. -- RESOLUTION: A dev not was added.
  • the explanation of the r and s value in the signature of the deployment is unclear. -- RESOLUTION: The text has been updated as to per his recommendation to be clearer.
  • the typing of some titles is inconsistent. -- RESOLUTION: The typing of the inconsistent titles has been updated.
  • the implementsERC165Interface(address,byte4) function has the view keyword but may update the ERC165 cache and thus the state. -- RESOLUTION: His suggestion to not update the cache and not modify the stat in the view function has been implemented.
  • The EIP uses STATICCALL and should mention EIP214 as a dependency. -- RESOLUTION: EIP214 has been added as a dependency
  • The rationale section is missing. -- RESOLUTION: The rational section has been added.
  • The EIP links to a "mutable" link of the implementation at jbaylina/ERC820 and it should be changed to an immutable link (including a commit hash for example). -- RESOLUTION: None, as it was answered, the actual code is in the EIP itself and should be immutable. The code in the referenced implementation is a copy of that code and should not be changed, however providing a link pointing to the master branch ensures the surrounding code such as examples is always up to date.

• @wighawag mentioned:

  • The motivation section could have more motivation: -- RESOLUTION: the motivation section was improved.
  • Allowing a proxy contract to implement an interface for another contract may not make sense and the registry should enforce the address for which an interface is implemented is an EOA (using code_size=0). -- RESOLUTION: None, the registry is intentionally designed to also allow existing contracts to have proxy contracts. This was answered in the comments.

• @thegostep mentioned:

  • The ManagerChanged in setManager(address,address) should always be emitted with the address of the new manager, even when the zero address is passed. -- RESOLUTION: None, this became a non-issue as we already removed the option to resolve the zero address to the sender as the new manager.

[fulldecent]

👍

I believe the authors have done an excellent job and faithfully followed the entire EIP process. Every review item I have seen has been respectfully handled and documented here. There should be no controversy in accepting this EIP as FINAL.

And for the subject matter of the EIP itself, it's not bad at all!

Shining example here. Good job!

[Arachnid]

The last call process revealed a number of changes, some of which were material. How confident are the authors that no further changes will be forthcoming?

[0xjac]

@Arachnid Fair question, sadly there is never a 100% guarantee.

We have thoroughly reviewed the standard and had other people do the same.
The majority of the changes are actually either related to style or typos.

However there were a few material changes:

• canImplementInterfaceForAddress(address,bytes32): This was not a significant change and it was mainly done for clarity and consistency.
  The order of parameters and their signification has been reviewed for all other public functions and they should all be consistent. While such changes to the registry cannot be made in the future there would not result in functional changes.

• interfaceHash(string) as external (instead of public): Similar to the previous change all the function access modifiers have been reviewed and the should be correct.

• implementsERC165Interface(address,byte4) does not change the state anymore: This is a specfic issue we were aware off. While not incorrect it was confusing and we had long discussions about it without ever being able to reach a consensus. In the end, @fulldecent's comments convinced us to change it. It is my belief that @fulldecent thoroughly reviewed the standard and the code of the registry and this issue is now fully solved.

• Remove the default value of msg.sender for the first parameter of setManager(address,address) when a zero-value is passed: We reviewed and discussed all functions which use this behavior and identified this function to be the only problematic one, where this behavior can result in unintended consequences.

I apologize for the delay I had some personal issues which came up and that I needed to take care of urgently, but I should be around until Christmas if you have further questions. I am also working towards allocating more of my time to blockchain-related work and the Ethereum community. 🙂

[0xjac]

After a discussion between @Arachnid and @jbaylina a new last call has been requested. Therefore I am closing this PR, see #1660.

[fulldecent]

Please link to the one for reference