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 820: Pseudo-introspection using a registry contract. #820

Open
jbaylina opened this Issue Jan 5, 2018 · 62 comments

Comments

Projects
None yet
@jbaylina
Contributor

jbaylina commented Jan 5, 2018

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.

@jbaylina jbaylina changed the title from EIP 812: Pseudo-introspection using a registry contract. to EIP 820: Pseudo-introspection using a registry contract. Jan 5, 2018

@stevenh512

This comment has been minimized.

stevenh512 commented Jan 15, 2018

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

This comment has been minimized.

Contributor

mcdee commented Jan 17, 2018

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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 30, 2018

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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 30, 2018

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

@hyperfekt

This comment has been minimized.

hyperfekt commented Jan 30, 2018

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

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 30, 2018

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.)

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 30, 2018

Now here's a crazy idea.

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

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 30, 2018

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

@hyperfekt

This comment has been minimized.

hyperfekt commented Jan 30, 2018

is there anything more that explains the calculations?

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

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 31, 2018

@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

This comment has been minimized.

Recmo commented Jan 31, 2018

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?

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 31, 2018

@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

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 31, 2018

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

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Jan 31, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 31, 2018

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

This comment has been minimized.

Recmo commented Jan 31, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Jan 31, 2018

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

This comment has been minimized.

Recmo commented Jan 31, 2018

@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).

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Feb 1, 2018

Yes, the attack is valid. Good fix.

@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Feb 1, 2018

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

@rstormsf

This comment has been minimized.

rstormsf commented Feb 21, 2018

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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Feb 23, 2018

@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;
    }
}
@jbaylina

This comment has been minimized.

Contributor

jbaylina commented Feb 24, 2018

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

Please review.

@mcdee

This comment has been minimized.

Contributor

mcdee commented Sep 21, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Sep 21, 2018

@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

This comment has been minimized.

Contributor

mcdee commented Sep 21, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Sep 21, 2018

@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

This comment has been minimized.

Contributor

mcdee commented Sep 21, 2018

@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?

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Sep 22, 2018

@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

This comment has been minimized.

Contributor

mcdee commented Sep 22, 2018

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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Sep 24, 2018

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.

@wighawag

This comment has been minimized.

Contributor

wighawag commented Oct 1, 2018

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.

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Oct 1, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Oct 8, 2018

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

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Oct 16, 2018

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?

@thegostep

This comment has been minimized.

thegostep commented Oct 22, 2018

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);
}
@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Oct 22, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Oct 22, 2018

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.

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Oct 23, 2018

@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.

@RyRy79261

This comment has been minimized.

RyRy79261 commented Oct 23, 2018

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);
    }```

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Oct 23, 2018

@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

This comment has been minimized.

Contributor

mcdee commented Oct 30, 2018

@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.

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Oct 31, 2018

@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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Nov 6, 2018

@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.)


@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Nov 6, 2018

@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).

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Nov 6, 2018

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.

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Nov 7, 2018

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

@jacquesd

This comment has been minimized.

Contributor

jacquesd commented Nov 7, 2018

@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).

@fulldecent

This comment has been minimized.

Contributor

fulldecent commented Nov 7, 2018

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

Can you please confirm:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment