ethereum/EIPs

EIP 820: Pseudo-introspection using a registry contract.

jbaylina opened this issue · 82 comments

Please, see: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-820.md for further discussion.


Preamble

EIP: 820
Title: Pseudo-introspection using a registry contract.
Author: Jordi Baylina <jordi@baylina.cat>
Type: StandardTrack
Category: ERC
Status: Draft
Created: 2018-01-05

Simple Summary

This standard defines a universal registry smart contract where any address (contract or regular account) can register which interface it implements and which smart contract is responsible for its implementation.

This standard keeps backwards compatibility with EIP-165

Abstract

This standard attempts to define a registry where smart contracts and regular accounts can publish which functionalities they implement.

The rest of the world can query this registry to ask if a specific address implements a given interface and which smart contract handles its implementation.

This registry can be deployed on any chain and will share the exact same address.

Interfaces where the last 28 bytes are 0 are considered EIP-165 interfaces, and this registry
will forward the call to the contract to see if they implement that interface.

This contract will act also as an EIP-165 cache in order to safe gas.

Motivation

There has been different approaches to define pseudo-introspection in the Ethereum. The first is EIP-165 which has the problem that it is not available for regular accounts to use. The second approach is EIP-672 which uses reverseENS. Using reverseENS, has two issues. First, it is unnecessarily complex, and second, ENS is still a centralized contract controlled by a multisig. This multisig, theoretically would be able to modify the system.

This standard is much simpler than EIP-672 and it is absolutely decentralized.

This standard also solves the problem of having different addresses for different chains.

Specification

The smart contract

pragma solidity 0.4.20;

interface ERC820ImplementerInterface {
    /// @notice Contracts that implement an interferce in behalf of another contract must return true
    /// @param addr Address that the contract woll implement the interface in behalf of
    /// @param interfaceHash keccak256 of the name of the interface
    /// @return ERC820_ACCEPT_MAGIC if the contract can implement the interface represented by
    ///  `ìnterfaceHash` in behalf of `addr`
    function canImplementInterfaceForAddress(address addr, bytes32 interfaceHash) view public returns(bytes32);
}

contract ERC820Registry {
    bytes4 constant InvalidID = 0xffffffff;
    bytes4 constant ERC165ID = 0x01ffc9a7;
    bytes32 constant ERC820_ACCEPT_MAGIC = keccak256("ERC820_ACCEPT_MAGIC");


    mapping (address => mapping(bytes32 => address)) interfaces;
    mapping (address => address) managers;
    mapping (address => mapping(bytes4 => bool)) erc165Cache;

    modifier canManage(address addr) {
        require(getManager(addr) == msg.sender);
        _;
    }


    event InterfaceImplementerSet(address indexed addr, bytes32 indexed interfaceHash, address indexed implementer);
    event ManagerChanged(address indexed addr, address indexed newManager);

    /// @notice Query the hash of an interface given a name
    /// @param interfaceName Name of the interfce
    function interfaceHash(string interfaceName) public pure returns(bytes32) {
        return keccak256(interfaceName);
    }

    /// @notice GetManager
    function getManager(address addr) public view returns(address) {
        // By default the manager of an address is the same address
        if (managers[addr] == 0) {
            return addr;
        } else {
            return managers[addr];
        }
    }

    /// @notice Sets an external `manager` that will be able to call `setInterfaceImplementer()`
    ///  on behalf of the address.
    /// @param addr Address that you are defining the manager for.
    /// @param newManager The address of the manager for the `addr` that will replace
    ///  the old one.  Set to 0x0 if you want to remove the manager.
    function setManager(address addr, address newManager) public canManage(addr) {
        managers[addr] = newManager == addr ? 0 : newManager;
        ManagerChanged(addr, newManager);
    }

    /// @notice Query if an address implements an interface and thru which contract
    /// @param addr Address that is being queried for the implementation of an interface
    /// @param iHash SHA3 of the name of the interface as a string
    ///  Example `web3.utils.sha3('ERC777Token`')`
    /// @return The address of the contract that implements a specific interface
    ///  or 0x0 if `addr` does not implement this interface
    function getInterfaceImplementer(address addr, bytes32 iHash) constant public returns (address) {
        if (isERC165Interface(iHash)) {
            bytes4 i165Hash = bytes4(iHash);
            return erc165InterfaceSupported(addr, i165Hash) ? addr : 0;
        }
        return interfaces[addr][iHash];
    }

    /// @notice Sets the contract that will handle a specific interface; only
    ///  the address itself or a `manager` defined for that address can set it
    /// @param addr Address that you want to define the interface for
    /// @param iHash SHA3 of the name of the interface as a string
    ///  For example `web3.utils.sha3('Ierc777')` for the Ierc777
    function setInterfaceImplementer(address addr, bytes32 iHash, address implementer) public canManage(addr)  {
        require(!isERC165Interface(iHash));
        if ((implementer != 0) && (implementer!=msg.sender)) {
            require(ERC820ImplementerInterface(implementer).canImplementInterfaceForAddress(addr, iHash)
                        == ERC820_ACCEPT_MAGIC);
        }
        interfaces[addr][iHash] = implementer;
        InterfaceImplementerSet(addr, iHash, implementer);
    }


/// ERC165 Specific

    function isERC165Interface(bytes32 iHash) internal pure returns (bool) {
        return iHash & 0x00000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF == 0;
    }

    function erc165InterfaceSupported(address _contract, bytes4 _interfaceId) constant public returns (bool) {
        if (!erc165Cache[_contract][_interfaceId]) {
            erc165UpdateCache(_contract, _interfaceId);
        }
        return interfaces[_contract][_interfaceId] != 0;
    }

    function erc165UpdateCache(address _contract, bytes4 _interfaceId) public {
        interfaces[_contract][_interfaceId] =
            erc165InterfaceSupported_NoCache(_contract, _interfaceId) ? _contract : 0;
        erc165Cache[_contract][_interfaceId] = true;
    }

    function erc165InterfaceSupported_NoCache(address _contract, bytes4 _interfaceId) public constant returns (bool) {
        uint256 success;
        uint256 result;

        (success, result) = noThrowCall(_contract, ERC165ID);
        if ((success==0)||(result==0)) {
            return false;
        }

        (success, result) = noThrowCall(_contract, InvalidID);
        if ((success==0)||(result!=0)) {
            return false;
        }

        (success, result) = noThrowCall(_contract, _interfaceId);
        if ((success==1)&&(result==1)) {
            return true;
        }
        return false;
    }

    function noThrowCall(address _contract, bytes4 _interfaceId) constant internal returns (uint256 success, uint256 result) {
        bytes4 erc165ID = ERC165ID;

        assembly {
                let x := mload(0x40)               // Find empty storage location using "free memory pointer"
                mstore(x, erc165ID)                // Place signature at begining of empty storage
                mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature

                success := staticcall(
                                    30000,         // 30k gas
                                    _contract,     // To addr
                                    x,             // Inputs are stored at location x
                                    0x08,          // Inputs are 8 bytes long
                                    x,             // Store output over input (saves space)
                                    0x20)          // Outputs are 32 bytes long

                result := mload(x)                 // Load the result
        }
    }
}

Raw transaction for deploying the smart contract on any chain

0xf908778085174876e800830c35008080b908246060604052341561000f57600080fd5b6108068061001e6000396000f30060606040526004361061008d5763ffffffff7c010000000000000000000000000000000000000000000000000000000060003504166329965a1d81146100925780633d584063146100bd578063571a1f66146100f85780635df8122f1461012457806365ba36c11461014957806390e47957146101ac578063aabbb8ca146101ec578063ddc23ddd1461020e575b600080fd5b341561009d57600080fd5b6100bb600160a060020a03600435811690602435906044351661023a565b005b34156100c857600080fd5b6100dc600160a060020a03600435166103ec565b604051600160a060020a03909116815260200160405180910390f35b341561010357600080fd5b6100bb600160a060020a0360043516600160e060020a031960243516610438565b341561012f57600080fd5b6100bb600160a060020a03600435811690602435166104c2565b341561015457600080fd5b61019a60046024813581810190830135806020601f8201819004810201604051908101604052818152929190602084018383808284375094965061057d95505050505050565b60405190815260200160405180910390f35b34156101b757600080fd5b6101d8600160a060020a0360043516600160e060020a0319602435166105e2565b604051901515815260200160405180910390f35b34156101f757600080fd5b6100dc600160a060020a0360043516602435610658565b341561021957600080fd5b6101d8600160a060020a0360043516600160e060020a0319602435166106b7565b8233600160a060020a031661024e826103ec565b600160a060020a03161461026157600080fd5b61026a8361076e565b1561027457600080fd5b600160a060020a0382161580159061029e575033600160a060020a031682600160a060020a031614155b15610373576040517f4552433832305f4143434550545f4d41474943000000000000000000000000008152601301604051908190039020600160a060020a03831663f008325086866000604051602001526040517c010000000000000000000000000000000000000000000000000000000063ffffffff8516028152600160a060020a0390921660048301526024820152604401602060405180830381600087803b151561034b57600080fd5b6102c65a03f1151561035c57600080fd5b505050604051805191909114905061037357600080fd5b600160a060020a0384811660008181526020818152604080832088845290915290819020805473ffffffffffffffffffffffffffffffffffffffff191693861693841790558591907f93baa6efbd2244243bfee6ce4cfdd1d04fc4c0e9a786abd3a41313bd352db153905160405180910390a450505050565b600160a060020a038082166000908152600160205260408120549091161515610416575080610433565b50600160a060020a03808216600090815260016020526040902054165b919050565b61044282826106b7565b61044d57600061044f565b815b600160a060020a03928316600081815260208181526040808320600160e060020a031996909616808452958252808320805473ffffffffffffffffffffffffffffffffffffffff19169590971694909417909555908152600284528181209281529190925220805460ff19166001179055565b8133600160a060020a03166104d6826103ec565b600160a060020a0316146104e957600080fd5b82600160a060020a031682600160a060020a031614610508578161050b565b60005b600160a060020a0384811660008181526001602052604090819020805473ffffffffffffffffffffffffffffffffffffffff191694841694909417909355908416917f605c2dbf762e5f7d60a546d42e7205dcb1b011ebc62a61736a57c9089d3a4350905160405180910390a3505050565b6000816040518082805190602001908083835b602083106105af5780518252601f199092019160209182019101610590565b6001836020036101000a038019825116818451161790925250505091909101925060409150505180910390209050919050565b600160a060020a0382166000908152600260209081526040808320600160e060020a03198516845290915281205460ff161515610623576106238383610438565b50600160a060020a03918216600090815260208181526040808320600160e060020a0319949094168352929052205416151590565b6000806106648361076e565b1561068957508161067584826105e2565b610680576000610682565b835b91506106b0565b600160a060020a038085166000908152602081815260408083208784529091529020541691505b5092915050565b600080806106e5857f01ffc9a700000000000000000000000000000000000000000000000000000000610790565b90925090508115806106f5575080155b156107035760009250610766565b61071585600160e060020a0319610790565b909250905081158061072657508015155b156107345760009250610766565b61073e8585610790565b90925090506001821480156107535750806001145b156107615760019250610766565b600092505b505092915050565b7bffffffffffffffffffffffffffffffffffffffffffffffffffffffff161590565b6000807f01ffc9a70000000000000000000000000000000000000000000000000000000060405181815284600482015260208160088389617530fa935080519250505092509290505600a165627a7a72305820b424185958879a1eef1cb7235bfd8ed607a7402b46853860e5343340925f028e00291ba079be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798a00aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

You can see the string of as at the end of the transaction. This is the s of the signature, meaning that its a deterministic by hand forced signature.

Deployment method

This contract is going to be deployed using the Nick's Method.

This method works as follows:

  1. Generate a transaction that deploys the contract from a new random account. This transaction must not use EIP-155 so it can work on any chain. This transaction needs to also have a relatively high gas price in order to be deployed in any chain. In this case, it's going to be 100Gwei.
  2. Set the v, r, s of the transaction signature to the following values:
    v: 27
    r: 0x79BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798
    s: 0x0aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    This nice s value is a random number generated deterministically by a human.
  3. We recover the sender of this transaction. We will have an account that can broadcast that transaction, but we also have the waranty that nobody knows the private key of that account.
  4. Send Ether to this deployment account.
  5. Broadcast the transaction.

This operation can be done in any chain, guaranteed that the contract address is going to always be the same and nobody will be able to mess up that address with a different contract.

Special registry deployment account

0x91c2b265ece9442ed28e3c4283652b1894dcdabb

This account is generated by reverse engineering it from it's signature for the transaction, in this way no one knows the private key, but it is known that it's the valid signer of the deployment transaction.

Deployed contract

0x991a1bcb077599290d7305493c9a630c20f8b798

The contract will have this address for every chain it is deployed to.

Interface name

Your interface name is hashed and sent to getInterfaceImplementer(). If you are writing a standard, it is best practice to explicitly state the interface name and link to this published EIP-820 so that other people don't have to come here to look up these rules.

If it's an approved EIP

The interface is named like ERC###XXXXX. The meaning of this interface is defined in the EIP specified. And XXX should be the name of the interface camelCase.

Examples:

sha3("ERC20Token")
sha3("ERC777Token")
sha3("ERC777TokensReceiver")
sha3("ERC777TokensSender")

EIP-165 compatible interfaces

Interfaces where the last 28bytes are 0, then this will be considered an EIP-165 interface.

Private user defined interface

This scheme is extensible. If you want to make up your own interface name and raise awareness to get other people to implement it and then check for those implementations, great! Have fun, but please do not conflict with the reserved designations above.

Backwards Compatibility

This standard is backwards compatible with EIP-165, as both methods can be implemented without conflicting one each other.

Test Cases

Please, check the repository https://github.com/jbaylina/eip820 for the full test suit.

Implementation

The implementation can be found in this repo: https://github.com/jbaylina/eip820

Copyright

Copyright and related rights waived via CC0.

I was a fan of EIP-672 (with the minor changes I made to preserve normal reverse resolution), but I really like this, it's a lot simpler.

I wonder if managers really needs to be its own mapping. I realized with EIP-672 that the same mechanism can be used to assign roles in situations where you want to enforce one address per role (think CryptoKitties, their contract has CEO, COO, and CFO roles). Instead of a separate mapping, an app could just use setInterfaceImplementation("role_manager", newManager) and the registry could use interfaces[addr][keccak256("role_manager")] to look it up. (The prefix "role_" was used to prevent collision in case a contract wants to register an interface called "manager", realistically any prefix could be used as long as it's unlikely to be confused with an interface name).

The above (at least for the manager role) could be abstracted away into the registry, so that managers(addr) returns interfaces[addr][keccak256("role_manager")] and changeManager(addr, newManager) sets interfaces[addr][keccak256("role_manager"}] = newManager.

mcdee commented

Should there be a standard, or at least a recommendation, for interface names? The example interface name Ierc777 requires people to remember the capitalisation and also confuses ERC with EIP. It also starts with 'I', which is implied by the definition of what the registry provides.

Stating that interface names should be lower-case, and those that come from an EIP should be of the format eipx e.g. eip777 might reduce confusion when attempting to check interfaces.

I am working on the competing ERC-165. We are suffering from one very specific problem: how to deal with these crazy DELEGATECALL contracts that may implement an interface today and stop implementing tomorrow. This EIP certainly is a solution to that.

I don't understand your deployment strategy to a specified address. Is this already documented, could you please link to that.

@fulldecent There's some source code linked here:
#777 (comment)

There is no need to choose a special name for the manager's interface. Simply set the hash directly.

function managerHash() public pure returns(bytes32) {
    return 0xffffffff;
}

Here is an implementation passing all test cases: jbaylina/ERC820#3

(MY FORK DOES NOT UPDATE THE README AND NEW CONTRACT HASH, IMPORTANT STEPS INDEED.)

Now here's a crazy idea.

What if 165 did the same thing is this but just removed the [addr] part?

@hyperfekt Thank you, and is there anything more that explains the calculations?

is there anything more that explains the calculations?

Pinging @Arachnid, since he came up with that technique.

@mcdee

Proposal for naming interfaces.

Interface name

Your interface name is hashed and sent to getInterfaceImplementer(). If you are writing a standard, it is best practice to explicitly state the interface name and link to this published EIP 820 so that other people don't have to come here to look up these rules.

If it's an approved EIP

The interface is named like erc20. The meaning of this interface is defined in the EIP specified.

If it's part of an approved EIP

The interface is named like erc721-ERC721Enumerable . The meaning of this interface is defined in the EIP specified. The interface name is defined in the standard.

If it's a draft EIP / on standards track

The interface is named erc20-e48d3ef where e48d3ef is a git commit hash in a pull request against https://github.com/ethereum/EIPs that includes sufficient details to implement the interface.

Part of a draft? erc721-1ca7dfb-ERC721Enumerable

Somebody posted a draft interface in a GitHub comment

Remember, comments are editable (retaining the same URL) and history is not retained. To be clear, EIP-820 doesn't help you much, you're really just playing around at this point.

Name it like erc20-https://github.com/ethereum/EIPs/issues/820#issue-286266121 based on the comment URL which describes your interface.

Part of a comment? erc20-https://github.com/ethereum/EIPs/issues/820#issue-286266121-ERC20Detailed

A function

You want to advertise support for an external Solidity function, but the details of the function are unspecified.

The interface is named like bob(bytes4) where bob is the function name and the function parameters are included in order (canonicalized, like int => int32) and separated by commas.

Something else

This scheme is extensible. If you want to make up your own interface name and raise awareness to get other people to implement it and then check for those implementations, great! Have fun, but please do not conflict with the reserved designations above.

recmo commented

What happens if I make a token contract, and then maliciously set the erc20 implementer to the Golem token contract. Will contracts using my token now unexpectedly call transfer on the Golem token?

@fulldecent
I would remove the drafts part. You will break the compatibility when the standard is defined.

Regarding the name, i like the interfaces to have readable names:

erc20-Token
erc777-Token
erc777-TokenHolder

@fulldecent Updated the proposal with your proposed changes without the drafts...

@recmo The manager of the golemContract would be the only one to change the implementation for his address. And in the case of the token interface, this does not apply.

The issue regarding drafts is, for example with 721, there are now 7+ unversioned and incompatible interfaces for ERC-721. If cryptokitties launched with "I support 721" then somebody using 721 interefaces will be disappointed when CK doesn't do it the way that is standardized.

recmo commented

@jbaylina Let me illustrate the security issue with a more explicit example. Consider the following hypothetical exchange that implements uses EIP820.sol to access the InterfaceImplementationRegistry:

contract SomeExchange is EIP820 {
    
    function settleTokens(address token, address from, address to, uint amount)
        internal
    {
        ERC20 erc20 = ERC20(interfaceAddr(token, "erc20-Token"));
        erc20.transferFrom(from, to, ammount);
    }
}

This is how EIP820 is intended to be used, correct?

We will attack this exchange. First, I deploy a reasonable token contract that implements EIP820:

contract ReasonableToken is Ownable, ERC20, EIP820 {
    
    function ReasonableToken()
        public
    {
        setInterfaceImplementation("erc20-Token", this);
        delegateManagement(owner());
    }
    
    // ...
}

Looks reasonable, right?

Now I get people to trade this token on the exchange. When the conditions are right, I make the following transaction (pseudo-code):

const reasonableToken = ReasonableToken(0x0123.....);
const zrxToken = ERC20(0xe41d2489571d322189246dafa5ebde1f4699f498);
const iir = InterfaceImplementationRegistry(0xa80366843213DFBE44307c7c4Ce4BcfC7A6437E5);

iir.setInterfaceImplementer(reasonableToken, keccak256("erc20-Token"), zrxToken);

This transaction succeed, because I'm a manager of the ReasonableToken interfaces.

After this, all settlements that where supposed to be made in ReasonableToken, are now made in ZRXToken! I set things up right, and now receive valuable ZRX token from people thinking they are selling me cheaper ReasonableToken. The use of ERC20 is just an example, this would work on any interface.

This works, because in setInterfaceImplementer I only need to manage the origin (canManage(addr)) contract, I don't need the manager role on the target implementer contract.

Note that my use of delegation/manager was only to obscure the attack a bit more. A similar attack can be done without it.

How to fix it: setInterFaceImplementer needs to verify that implementer is intended to be the implementer for addr:

contract EIP820Implementer {
    function implementsFor() public returns (address);
}
    function setInterfaceImplementer(address addr, bytes32 iHash, address implementer) public canManage(addr)  {
        require(EIP820Implementer(implementer).implementsFor() == addr);
        interfaces[addr][iHash] = implementer;
        InterfaceImplementerSet(addr, iHash, implementer);
    }

For extra safety it should also verify that it intends to implement the requested interface.

Alternative: canManage(addr) canManage(implementer) would also work.

Ah... no. There is no contract that needs to inherit from ERC-820. ERC-820 is not an interface. It is just a specific contract on the blockchain.

recmo commented

@fulldecent I'm using EIP820.sol, which to me looks like it's intended to be inherited from. (I think you might confuse it with InterfaceImplementationRegistry.sol, which is the singleton registery that has its address hardcoded in the EIP820.sol. The naming is a bit unfortunate.).

I can rewrite the attack to work without inheritance, if you like. (I also updated my previous comment to be a bit more clear about this).

Yes, the attack is valid. Good fix.

@recmo @fulldecent Updated the proposal with your suggestion. Please review.

jbaylina/ERC820#5

https://github.com/jbaylina/eip820/blob/master/contracts/ExampleImplementer.sol#L5

pragma solidity ^0.4.18;

contract ExampleImplementer {
    function canImplementInterfaceForAddress(address addr, bytes32 interfaceHash) view public returns(bool) {
        return true;
    }
}

If I unlock the fallback function and provide some default behavior, then it would break those checks because of strange behavior by design in solidity if a method does not exist it will instead execute the fallback function, and if the fallback function does not raise an exception it will return 1 causing the check to pass.

The only solution that comes to my mind is to use some magic numbers. Maybe we should return uint which should be more than 0.

@rstormsf I have heard this before too, but do you know of the canonical source which explains this further. I would just like to double check before we go thinking we solved the problem.

contract ExampleImplementer {
    function canImplementInterfaceForAddress(address addr, bytes32 interfaceHash) view public returns(uint8) {
        return 42;
    }
}

@fulldecent @rstormsf Just added compatibility with EIP165 and the magic return.

Please review.

This magic value is much better than 42. Because the function signature may collide with other functions and 42 will collide with other people that choose 42.

Just open the PR #906 to be merged/approved.

The problem of using an external dependence, so things like "address of contract" and "trustfulness of its contents" are a problem, but from what I understood, ERC820Registry address will be derived from the code of contract, which is a brilliant way of solving this problem.

Having the external dependency trust solved this can be incorporated into Solidity or other high level ethereum programming languages, where the compiler can deploy this libraries when they are needed (not yet deployed).

I think that if ERC820 is implemented by solidity, than #777 would be a really good interface.
I need to learn more about the "pseudo implementation" of TokenReceiver and TokenSender, which seems to make solidity more abstract-able in terms of objective programming. This all would be powerful for interface abstraction, and ERC777 would help as showing first use case of it.

I'm quite confused by the Deployment Method above. What is the point of this? Is the point to create the registry with an account that no one holds the associated private key for? If so, it would be good to clearly define why that is needed.

If that's not the case, using

contract ERC820Implementer {
    ...
    IERC820Registry eip820Registry;
    function ERC820Implementer (address _erc820RegistryAddr)  {
        erc820Registry = IERC820Registry(_erc820RegistryAddr);
        ...
    }
    ...
}
contract ERC820ImplementerExample {
    ...
    function ERC820ImplementerExample(address _erc820RegistryAddr) ERC820Implementer (_erc820RegistryAddr) {
         ...
    }
    ...
}

along with:

await deployer.deploy(ERC820Registry);
await deployer.deploy(ERC820ImplementerExample, ERC820Registry.address);

this appears to be enough to accomplish the functionality described in this erc, but maybe not the features.

I imagine it's not the case that it's a good idea to deploy this with an address with a known key pair. But if that's the case, can we expand on why?

mcdee commented

@jbaylina the EIP-165 compatibility is problematic, in that the caching causes constant functions to write data when updating the cache.

Does the EIP-165 lookup need a cache? Given that a non-caching version would be truly constant I don't see the benefit of having a cache in this situation.

The example shows:

sha3("ERC777Token")
sha3("ERC777TokensReceiver")
sha3("ERC777TokensSender")

But interfaces with those names don't exist. (Thank you Stephane Gosselin.)

Has anyone taken a stab at using 820 for external storage akin to https://medium.com/cardstack/upgradable-contracts-in-solidity-d5af87f0f913

I updated the contract so that it accepts 0x0 as the addr. This is equivalent to msg.sender. This way, we can instruct users of the multisigs easilly by giving them a single Data to enable an interface. (For example to allow multisigs to accept ERC777 tokens).

Although I have no issues following along with the 820 repository, this page (and this one) offers up a few confusing things that are out of sync with the repository:

Official deployment address:
From the repository: 0xda83F1A8e9506Ec973DBfD6090fE17EB91eE7BF4
here: 0x91c2b265ece9442ed28e3c4283652b1894dcdabb

Deployed contract:
From the repository: 0xa691627805d5FAE718381ED95E04d00E20a1fea6
here: 0x991a1bcb077599290d7305493c9a630c20f8b798

AFAICT the repository has the latest bit, and this page just needs to be updated :)

To where may I send pull requests for such? (I am not sure how you are handling updates to https://eips.ethereum.org/EIPS/eip-820 or here)

Can you clarify which one we should be using? ( There are 2 contracts deployed )

0xjac commented

@jaycenhorton @gitcnd we are still working on ERC820 and because of the nature of the deployment method used, any change to the registry contract implies a change of address.

The addresses you mention are probably old versions deployed for testing purposes and should not be used.

The actual address is indicated in the latest draft of the standard at https://eips.ethereum.org/EIPS/eip-820 as mentioned on top of the issue:

Please, see: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-820.md

However, since the standard is still a draft, there is no guarantee that it the final address for the registry contract. @jbaylina and I are working on finalizing the standard now. Hopefully, it will be approved in a few weeks.

NOTICE: This EIP entered Last call.

@jbaylina Please explicitly state the review-end-date for +2 weeks.

Review notes coming.

Looks good, here is my complete review of the current version.


EIP820 vs EIP-820

The contract includes ASCII art which refers to "ERC820" however, the preferred styling would be "ERC-820" if following the convention of in the README.md at https://github.com/ethereum/EIPs


Implicit zero address meaning

The implementation has several locations where an address of zero has the meaning of "the sender".

/// (If _addr == 0 then msg.sender is assumed.)
/// @param _addr Address to define the interface for. (If _addr == 0 then msg.sender is assumed.)
/// @param _addr Address for which to set the new manager. (If _addr == 0 then msg.sender is assumed.)
_addr: Address to define the interface for (if _addr == 0 them msg.sender: is assumed)
_addr: Address being queried for the implementer of an interface. (If _addr == 0 them msg.sender is assumed.)

I question the value of this implicit default and it has not been explained as a design decision. Passing a value of zero costs just as much gas as passing the current contract's address. Also, the current contract's address is simple to get during a contract construction as shown by this screenshot.

screen shot 2018-09-21 at 3 38 07 pm

The downside of an implicit default is that it may be used by accident or make cause an unintended result.


Argument ordering: canImplementInterfaceForAddress(address addr, bytes32 interfaceHash)

A plain reading of this function name, canImplementInterfaceForAddress, would expect the arguments to be ordered as interface then address.


ERC-165

This specific builds onto of ERC-165. Please include ERC-165 as a "requires" as per the EIP metadata field.


Authorship of cache

@jbaylina and I both designed on the caching implementation included, it was originally part of ERC-165 but we agreed to remove it and put it here.

I don't remember who wrote that part of the code. But if it is me, please provide a citation or list me as a co-author, whichever you think is appropriate. Either way, I am very proud of this project, and glad it got to where it is!


Add @dev note

function noThrowCall(address _contract, bytes4 _interfaceId)

Best practice is to add a @dev note, at least, for all internal functions.


Clarify intent of

The strings of 820's at the end of the transaction are the r and s of the signature. From this pattern, one can clearly deduce that it is a deterministic signature generated by a human.

Currently this is a true statement. But the value may be lost on some readers. Consider:

The strings of 820's at the end of the transaction are the r and s of the signature. From this pattern, one can clearly deduce that it is a deterministic signature generated by a human. This allows you to confirm that nobody knows the private key for the account that will deploy this contract.

or similar.


Consistent type styling

Some headings use title case and some use sentence case. Convention from EIP-1 is to use title case for headings https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md


Modifiable view

NOTE: This function may modify the state when updating the cache. However, this function must have the view modifier since getInterfaceImplementer also calls it. If called from within a transaction, the ERC165 cache is updated.

This caveat breaks an assumption of the Ethereum ABI. And it will cause calls to getInterfaceImplementer to throw unexpectedly.

So sorry, I know I was involved in the original design of this part, but I was wrong. Here is a better approach:

    /// @notice Checks whether a contract implements an ERC165 interface or not.
    /// The result is cached. If the cache is out of date, it must be updated by calling `updateERC165Cache`.
    /// @param _contract Address of the contract to check.
    /// @param _interfaceId ERC165 interface to check.
    /// @return `true` if `_contract` implements `_interfaceId`, false otherwise.
    /// @dev This function may modify the state when updating the cache. However, this function must have the `view`
    /// modifier since `getInterfaceImplementer` also calls it. If called from within a transaction, the ERC165 cache
    /// is updated.
    function implementsERC165Interface(address _contract, bytes4 _interfaceId) public view returns (bool) {
        if (erc165Cached[_contract][_interfaceId]) {
            return interfaces[_contract][_interfaceId] == _contract;
        }
        return implementsERC165InterfaceNoCache(_contract, _interfaceId);
    }

    /// @notice Updates the cache with whether the contract implements an ERC165 interface or not.
    /// @param _contract Address of the contract for which to update the cache.
    /// @param _interfaceId ERC165 interface for which to update the cache.
    function updateERC165Cache(address _contract, bytes4 _interfaceId) public {
        interfaces[_contract][_interfaceId] = implementsERC165InterfaceNoCache(_contract, _interfaceId) ? _contract : 0;
        erc165Cached[_contract][_interfaceId] = true;
    }

    /// @notice Checks whether a contract implements an ERC165 interface or not without using nor updating the cache.
    /// @param _contract Address of the contract to check.
    /// @param _interfaceId ERC165 interface to check.
    /// @return `true` if `_contract` implements `_interfaceId`, false otherwise.
    function implementsERC165InterfaceNoCache(address _contract, bytes4 _interfaceId) public view returns (bool) {
        uint256 success;
        uint256 result;

        (success, result) = noThrowCall(_contract, ERC165ID);
        if (success == 0 || result == 0) {
            return false;
        }

        (success, result) = noThrowCall(_contract, INVALID_ID);
        if (success == 0 || result != 0) {
            return false;
        }

        (success, result) = noThrowCall(_contract, _interfaceId);
        if (success == 1 && result == 1) {
            return true;
        }
        return false;
    }

Then no such caveat is necessary.


STATICCALL

This EIP references the STATICCALL opcode (and not just indirectly through ERC-165) which is introduced in EIP-214 so this EIP should be updated to "require" that EIP.

Related update --> #1440


The Rationale section is missing.

Specified at https://github.com/ethereum/EIPs/blob/master/eip-X.md#rationale

This is where some design decisions can be detailed.

mcdee commented

@fulldecent

@jbaylina and I both designed on the caching implementation included, it was originally part of ERC-165 but we agreed to remove it and put it here.

As mentioned previously, at current getInterfaceImplementer() is a constant function that attempts to update state. This is at best bad design and at worst could result in an illegal contract if ever constant is enforced by the compiler.

@mcdee Responding to your other comment much earlier -- #820 (comment)

One benefit of the cache is that any call to ERC-820 (if properly configured and cache is warm) will only require one SLOAD and no CALL, which keeps it cheap.

mcdee commented

@fulldecent

One benefit of the cache is that any call to ERC-820 (if properly configured and cache is warm) will only require one SLOAD and no CALL, which keeps it cheap.

Not for the poor transaction that has to populate the cache.

But regardless of relative costs, the big issue remains that the function is marked constant but updates state.

@mcdee Please see the proposal at #820 (comment) If those changes are implemented, then do you agree that the cache has value?

This requires one person to pay for the SSTORE and then going forward everybody gets SLOAD instead of CALL * 3. And the costs are predictable versus the Russian roulette of the current version.

mcdee commented

@fulldecent

The problem with caching is always consistency. I see in the comment for the code:

If the cache is out of date, it must be updated by calling updateERC165Cache.

so it sounds like this implementation punts on consistency. Are the potential gas cost savings really worth this function possibly returning stale information?

@mcdee CryptoKitties has been out one year and changed its ERC-165 status only once (implemented metadata for ERC721, sort of). There were 40,000+ kitty transfers in the past 7 days. This situation seems worth it to use the cache.

mcdee commented

There were 40,000+ kitty transfers in the past 7 days.

But they don't use ERC-820. The only situation where this is an optimisation is where a contract asks the ERC-820 contract for interfaces registered on the ERC-165 contract.

This situation seems worth it to use the cache.

It always does, until a consistency issue causes an bad transaction result.

Having or not having the cache makes no difference to the operation of the contract outside of the cache potentially returning stale data. Publishing a standard with optimisations that cause a potential security issue seems to me to be a bad idea.

We are talking about ERC-165 implementation. This means supporting or not supporting a certain interface. The worst-case outcome of a stale result is that calls to the contract will throw because it stopped supporting an interface.

^ If you can find a concrete security issue with that then I can find a worse security issue with ERC-820.

If it is important to a contract owner that the ERC-820 singleton will not have stale data then they can call the update function when they make a change. If people don't care about putting data into the ERC-820 singleton then there is no point in having any discussion about EIP 820: Pseudo-introspection using a registry contract.

I like the idea of this proposal but the motivation section could elaborate more. In particular, it does not mention the motivation behind allowing another contract implementing the interface for another address. I guess the main reason was for supporting externally owned address but this is not clear from the motivation section that it was the purpose of such delegation.

I can also see this being beneficial for some cases where you would want your existing contract to support a later standard by creating an "adapter" contract that support the new standard. Though I am not sure it is a good idea. Should we actually prevent this use case by enforcing addr to be externally owned (contract code = 0)?

Unfortunately the role of an adapter is not simple for standard that requires events to be triggered for specific state transition. Indeed, when implementing such standard, an adapter contract would need to listen for the changes happening via the old contract interfaces, including the past changes.

I am not sure if such case where part of the motivation but adding the use cases in mind in the description of the proposal might make it clearer for the outsiders which might think it can apply to all use case.

0xjac commented

@fulldecent Thank you very much for your detailed feedback. I met with @jbaylina to discuss and adapt 820 based on your comments.

review-end-date

I'll admit I missed that one. I will add it. However i would suggest to make this more explicit in the EIP1. It's only listed in the header preamble section. I would include a note in the work flow section under last call that this must be set at this time.

EIP820 v. EIP-820

The readme does mention EIP-1, however we actually believe this should be changed to EIP1.
Not putting the dash is more consistent, both with Solidity code where the dash is not valid syntax (we have to use contract ERC820Registry {...}) and with EIP(-)1 which itself refers
to (in order) EIP5, EIP101, EIP90, EIP86 EIP8, EIP59, EIP6, ERC20, ERC26, ERC137, ERC67, EIP82, EIP75, and EIP85, all without dash.

I would be interested to see this syntax normalized across all EIPs, in addition, it would be nice to recommend that all EIPs refereed in this format are turned into links. (This can be done easily in markdown using [EIPXYZ] in the text and [EIPXYZ]: <link> at the bottom of the document.)

Default (zero address) as address of the sender.

You are correct, we do need to explain this implicit default and the rationale behind it. And it is true that you can easily get the address of the contract in a constructor or any function with the this keyword.

However there are valid use cases where not having to know or use the address explicitly is useful. As an example, consider multisigs which need to set an interface. The default address to simplify the instructions for the users of the multisig. They can just approve a transaction with constant data, rather than constructing the data with the address of their multisig instance.

We will add a note explaining this in the standard.

Regarding the downside of using this by default, the risks are minimal and in any case you may always undo the change by setting a new implementer. The only case where this might be an issue is in setManager and we will remove it from this function.

Argument ordering: canImplementInterfaceForAddress(address addr, bytes32 interfaceHash)

Good catch, we will swap the arguments around as per your suggestion.

ERC165 Requirement

Good catch, we will add that ERC820 requires EIP165

Authorship of cache

I was not aware of this fact when I did a cleanup of the EIP but we will happily mention you for the work you did, especially considering the modification you suggested (see below).

Add @dev note

We will add them!

Clarify intent of (the deployment transaction signature)

Thanks, we will clarify this section based on your suggestion.

Consistent type styling

Thanks, we will convert all titles to title case.

Modifiable view

We looked at both the current implementation and your suggestion. The current implementation is more functional but yes this potentially modifiable view is ugly. Your solution is much cleaner but less functional (since you have to explicitly and manually call the cache).

However, we thought long and hard and you convinced us. We will use your implementation (and credit you for it of course 😉) and we will describe the process in the doc and the standard on how one should explicitly call the updateERC165Cache function.

STATICCALL

We will add the require for EIP214.

The Rationale section is missing.

Oops! Our bad we will definitely add it.

@wighawag Thank you for your feedback.

Regarding contracts implementing interfaces for other contracts, the main rationale was to both allow externally owned addresses to have implementations and for some existing contracts such as multisig to not be redeployed. They can add extra features in a separate contract.

For this reason, I would not prevent contracts from having other contracts as implementers. In addition when the address and the implementer differ, the registry will check with the implementer if it implements the interface for that address by calling canImplementInterfaceForAddress. Thus limiting abuses and accidents.

We will detail it more in the motivation and/or in the missing rationale section.

@jacquesd Detail is added to EIP-1 / (EIP1?) in #1477

It seems that the review date has ended and there is outstanding work to do. May I please request that this review be ended and status reverted to DRAFT?

Proposed modification to the ManagerChanged event inside of setManager to never emit the 0 address as the new manager.

function setManager(address addr, address newManager) public canManage(addr) {
    managers[addr] = newManager == addr ? 0 : newManager;
    ManagerChanged(addr, newManager == address(0) ? addr : newManager);
}
0xjac commented

@thegostep Thanks for your suggestion, however based on @fulldecent's feedback we will actually remove the implicte default 0 address for newManager.

@fulldecent I have implemented all your changes and I will update the standard shortly I just need to compute the new address. @jbaylina and I were thinking of extending the last call status by one week from the moment I publish the update to give everyone more time to finalize the review.

Because the last call date has already passed, I would request that this be reverted to draft and a brand new last call (14 days) begin.

This is mainly procedural. But I do think we should set a good example for future projects by respecting the procedure if it makes sense.

0xjac commented

@fulldecent you're right I have update the standard with all of your feed back and I put the new last call end date for two weeks from now.

I'm intending on implementing an ERC777 with 2 specific contracts, these contracts need to implement certain functions as the interaction requires the burning of tokens.

In reading the various iterations of 820 and its discussions, am I right in thinking addresses that have been registered in the registry that implement an interface, do so specifically for my account as the msg.sender.

So am I right in thinking that in order to verify if an address implements a specific interface, I need the requesting contract to have my address as part of the params.

Or is it that the contract that would query the interface should be set to a manager and have a function to interact with the 820 registry in order to set the entry for that contract?
CC @jbaylina, @fulldecent @jacquesd

    /// Only the manager defined for that address can set it.
    /// (Each address is the manager for itself until it sets a new manager.)
    /// @param _addr Address to define the interface for. (If `_addr == 0` then `msg.sender` is assumed.)
    /// @param _interfaceHash keccak256 hash of the name of the interface as a string.
    /// For example, `web3.utils.keccak256('ERC777TokensRecipient')` for the `ERC777TokensRecipient` interface.
    function setInterfaceImplementer(address _addr, bytes32 _interfaceHash, address _implementer) external {
        address addr = _addr == 0 ? msg.sender : _addr;
        require(getManager(addr) == msg.sender, "Not the manager");

        require(!isERC165Interface(_interfaceHash), "Must not be a ERC165 hash");
        if (_implementer != 0 && _implementer != msg.sender) {
            require(
                ERC820ImplementerInterface(_implementer)
                    .canImplementInterfaceForAddress(_interfaceHash, addr) == ERC820_ACCEPT_MAGIC,
                "Does not implement the interface"
            );
        }
        interfaces[addr][_interfaceHash] = _implementer;
        emit InterfaceImplementerSet(addr, _interfaceHash, _implementer);
    }```

0xjac commented

@RyRy79261

I right in thinking addresses that have been registered in the registry that implement an interface, do so specifically for my account as the msg.sender.

No, tt does so for the address passed as first parameter to setInterfaceImplementer. However only the manage of that address (which default to the address itself) can call that function successfully.

So am I right in thinking that in order to verify if an address implements a specific interface, I need the requesting contract to have my address as part of the params.

If an address implements a specific interface for some other address, then when registering the implement, the registry will call the implementer's canImplementInterfaceForAddress(_interfaceHash, _addr) function. That function must be present and return ERC820_ACCEPT_MAGIC if the implementer implement the interface for _addr. The mechanism of how the implementer decides if it implements the interface for the _addr is up to the implementer. It can store the address(es) for which it implements the interface or accept for any address.

Or is it that the contract that would query the interface should be set to a manager and have a function to interact with the 820 registry in order to set the entry for that contract?

When you say "the contract that would query the interface", are you talking about the implementer or the address for which the implementer implements an interface?
In any case every address is its own manager and thus can call setInterfaceImplementer for itself. In the case of a contract, the contract can either call setInterfaceImplementer for itself or call setManager and set another address which can then call setInterfaceImplementer on the contract's behalf.

If you need some help, I suggest you have a look at chapter 6 of my master thesis which provides more explanations about the ERC820 registry. It's under 15 pages with many sequences diagrams to illustrate various registration use cases.

mcdee commented

@jacquesd please can you sort out the reference implementation node module erc820 so that it matches the updated spec? Specifically, at current the arguments for canImplementInterfaceForAddress() are not the same as in the spec and the deployment address of the contract is incorrect.

0xjac commented

@mcdee yes, there is a PR pending (jbaylina/ERC820#17) with the updates. As soon as @jbaylina has time we'll merge it and update the npm package. If you need the registry urgently in the mean time just use my branch.

This should be solved by the end of the day or tomorrow.

@jacquesd regarding EIP naming. I have started that discussion explicitly at #1464 I hope there can be a resolution and consistency can be achieved. In no case should that concern hold up this proposal.


I have reviewed the current version da18d5e and all of my review notes per above have been fully addressed.

Anybody may review the complete set of changes made during the Last Call review by checking https://github.com/ethereum/EIPs/compare/48a426f..master#diff-a83ad54135690eb718b9a7261ea5f479


I have reviewed and validated the deployment signed transaction .

The EIP transaction is what was actually deployed to main net // confirm here https://etherscan.io/getRawTx?tx=0x196902ba04ced1d58cf488f5439402b5b04355e86e9430aa369baf0802a620f9

I have complied the EIP source code and my result is comparable to the EIP signed transaction byte code.

screen shot 2018-11-06 at 1 17 25 am

screen shot 2018-11-06 at 1 17 30 am


NEW ISSUE:

The EIP links to the contract source code at the mutable URL: https://github.com/jbaylina/ERC820/blob/master/contracts/ERC820Registry.sol

Instead it should point to an immutable URL including a specific commit reference.

The risk is that the referenced URL may change after publishing.

And in fact the referenced URL is already out of date with the code in the EIP. (I sent a PR to fix it.)


0xjac commented

@fulldecent

thanks I'll have a look at your proposal.


I can confirm that:

⚠️ The ERC820 registry address is 0x820Dd9AE49737695a0EA027670cB2E9cdc5d4E77 ⚠️
(deployed at 0x196...0f9).


Regarding the link, jbaylina/ERC820 contains the code for development and convienence, however the entire code for 820 is contained within the EIP.
Therefore I would keep the URL here as is. The registry code will not change but the code around might, hence if someone clicks on the link and navigate to other files they will always see the latest version of the code.

I don't see your PR but I cannot merge PRs on this repo, I'm not part of EF and not a maintainer. But I've updated the source code in the EIP, sadly it looks like even I have trouble having the PR merged (see #1557).

0xjac commented

I forgot to add, the address change was just for cosmetic reasons, there was a superfluous white space and removing just changes the address, but the code is exactly the same and it is verified on etherscan.

@jacquesd The PR is jbaylina/ERC820#18 on the ERC820 repo.

0xjac commented

@fulldecent thanks, during a final reading of the standard I noticed some comments were inaccurate. I updated them both in the ERC820 and the EIPs repo and this means the address and IV changed again.

Unfortunately there seems to be an issue with the EIPs bot not merging my changes. 🙁
In the meantime you can have a look at the changes here (EIP) and here (ERC820).

I'm also happy to extend the last call by a couple days to compensate for the technical issues and the give you time to look at those comments. Hopefully, this should take very little time as it's just a few comments, the bulk of the changes is updating the address and deployment transaction (whose bytecode is the same, only the metadata hash changes).

Damn, I couldn't get one commit in there, could I...

Can you please confirm:

mcdee commented

This has passed the last call review date as per https://github.com/ethereum/EIPs/blob/master/EIPS/eip-820.md - can this now be moved to final?

0xjac commented

@fulldecent I can confirm that:

edit 1: The erc820 library with the correct contract has been published to npm under version 0.0.24. However require('erc820') is not working because of a small issue with a dependency. I expect this to be solved by the end of the day and we will publish version 0.0.25 with the fix.
edit 2:: The erc820 library has been updated now to version 0.0.25. It contains the correct contract and the dependencies issues have been resolved such that require('erc820') now works properly. Thank you @jbaylina for the update!

But don't trust, verify (again).

I'm sorry about the commit, however any commit you can put there is appreciated. I think a lot of the code around the registry can be cleaned up and improved. If you want, (or anyone really, jbaylina/ERC777AcceptAny is also in need of an update (see with @jbaylina for details).

@mcdee thanks for your comment. The date is was indeed past however I had some tiny changes to be added and due to some bug with Github (I'm currently talking to Github support about it), my PRs to ethereum/EIPs are not being merged... I discussed with @Souptacular who kindly merged my pull request and he agreed to my suggestion of extending the last call deadline by a couple days to give a last chance for anyone to look at the changes. It's only a small comment but it looks big as the raw tx needs to be regenerated. In any case a couple days should be enough.

⚠️ The new review end date is 2018-11-21. ⚠️

0xjac commented

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

For this I would need some assistance for @Arachnid or @Souptacular to merge #1612.

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.

@jacquesd 0xjac, thanks a lot. Are all merges done ? could you move 820 to final status or did you find improvements ? Warm regards

0xjac commented

@BlueRoss No, I did not find improvements and I did not get further feedback. I was silent for the last 2 weeks for personal reasons but I am now allocating some time again to finalize this.

There is already a PR to finalize 820 at #1612. I will answer @Arachnid last comment which I missed and update the PR.

0xjac commented

After a discussion between @Arachnid and @jbaylina, a new last call for ERC820 has been requested, see #1660.

This last call ends in two days.

C'mon people, nothing to say?

0xjac commented

@fulldecent apparently not... Maybe everything was said during the previous last calls.

In any case, since there are no changes and the last call period has ended. I would like to move ERC820 to Final. Please see #1677.

I was trying to use this with Solidity 0.5, and discovered what may be an incompatibility between the EIP165-compatible code here and the new Solidity compiler.
It looks like the new solidity compiler enforces padding of call arguments, so a test-compile of e.g. TestERC165.sol results in code for supportsInterface(bytes4 interfaceID) which checks lt(sub(calldatasize,0x4),0x20) or reverts. The problem seems to be that noThrowCall sets the call size to be 0x8 bytes, which is technically sufficient, but makes the called-contract revert with the new compiler. I tried modifying noThrowCall locally to set the call size to 0x24 bytes, and then the test passes.
I don't use the ERC165 compatibility, we're past the last call date, this is arguably a problem with Solidity more than this specification, and it's entirely possible my analysis is completely wrong, but I wanted to at least bring awareness to the issue.

ABORT.

Confirming that we should update this.

There is a PR open on ERC-165 that we should accept. I lost the link and still searching for it.

@fulldecent It's talking about this bug #1640

In short, the Solidity >= 5 compiler forces all methods to validate that the input is valid (4 bytes + N Parameters * 32 bytes).

    function noThrowCall(address _contract, bytes4 _interfaceId) constant internal returns (uint256 success, uint256 result) {
        bytes4 erc165ID = ERC165ID;

        assembly {
                let x := mload(0x40)               // Find empty storage location using "free memory pointer"
                mstore(x, erc165ID)                // Place signature at begining of empty storage
                mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature

                success := staticcall(
                                    30000,         // 30k gas
                                    _contract,     // To addr
                                    x,             // Inputs are stored at location x
                                    0x08,          // Inputs are 8 bytes long
                                    x,             // Store output over input (saves space)
                                    0x20)          // Outputs are 32 bytes long

                result := mload(x)                 // Load the result
        }
    }

That call would never make a success when interacting with a contract compiled with Solidity >= 5, because there is no way of declaring a method that accepts an input of 8 bytes.

Replacing the call with an input of 36 bytes would fix the bug.

Now that the EIP has been updated to Final - is the Registry address provided on https://github.com/jbaylina/ERC820 locked in to be the final registry? Or are there still outstanding changes awaiting deployment?

0xjac commented

@thorvald @Agusx1211 thank you for spotting and explaining that. It is indeed a bit late but it is nonetheless important.
After some thoughts and multiple discussions, since the standard is still newly approved and there are no transaction on mainnet,
@jbaylina and I have decided to update the registry and redeploy the contract (this obviously implies a change of address) with the fix.

To avoid confusion the following procedure will be followed:

  1. EIP820 will be superseded (see EIP-1).
  2. EIP820a will contain the fix and be deployed.
  3. EIP820a will go through the standard 2 weeks last call.
  4. EIP777 (see #777) will go through its 2 weeks last call (soon after the start of the EIP820a last call).

This procedure was discussed with @jbaylina and @Arachnid.

@thorvald @Agusx1211 @fulldecent (and anyone else who is interested, maybe @mcdee?) I would kindly ask you to take a look at the changes to the registry. You can have a look online, or in case that link does not work, the two files are:

I will start the procedure and update the EIPS this weekend. Any feedback is appreciated.


@alsco77 The address at https://github.com/jbaylina/ERC820 the final one for ERC820, however from what is mentioned above, ERC820 will be superseded by ERC820a and the address will change.

Thank you for moving this along. Can you please paste a new diff link, that one is broken.

I don't know if a as a suffix is sufficient to distinguish it from the original. Would anybody entertain 820 and 820b? There is something about the letter a, which makes it sound like it is the first one which makes the original 820 sound like it /is/ 820a. This is a stupid comment so feel free to ignore it.

Regarding the process, yes, looks great.

I would recommend a few days buffer for 777. Just because I have seen some EIPs leave "two week review" slightly after two weeks.

In the future I might advocate against dependent EIPs running last call in parallel. But this instance seems to be particularly lower risk.

mcdee commented

If the altered contract is going to have a different designation I'd rather it had its own number than a suffix.

I'm happy with a new number too. I see little mention of 820 in the wild and using a new number is not likely to lead to confusion. Now, 777, that number is more important and it is unaffected by this change.

0xjac commented

@fulldecent The diff link has been updated and you can find it here too. Hopefully it should work this time... For the ERC777 last call there will be a buffer. I don't know of how much, that depends on how fast I can wrap up ERC777. But my comment was merely there to indicate that it will not take 2 weeks for ERC820 and another 2 weeks after for ERC777 but the last calls will be partially stacked.

@fulldecent @mcdee Regarding the 820a number, I disagree with a change of number. First we are not creating a new EIP, just fixing an incompatibility introduced by Solidity. Some people actually have been talking (at least to me) about 820. Having two EIPS numbers might be confusing as to where to look and which one to use.

Secondly, changing the number means quite a few changes across the EIP text and the code base for no gain other than having a new number. As @fulldecent mentioned we are moving this along and changing the number gain now everywhere will add days of work.

Now that Constantinople is happening, is there any appetite to change the 820 deployment mechanism to the more robust eip-1014 format?

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.