EIP 1820: Pseudo-introspection Registry Contract

[0xjac]

New EIP: Pseudo-introspection Registry Contract

This is a replacement for 820.

[chompomonim]

How about using CREATE2 as a deployment method with predictable address instead of currently used one? I can prepare PR, if you agree.

[jbaylina]

This would be good idea if there is a standarized Factory contract. But in the while I think it's better to stick to the current method.

[chompomonim]

standarized Factory contract

Sounds like idea for hobby project and one more EIP ;)

[0xjac]

@Arachnid, @cdetrio, @Souptacular, @vbuterin, @nicksavers, @wanderer, may I ask one of you to merge this draft. It is a replacement for EIP820 which was broken by the Solidity 0.5 update. See #820 and #1758 for more information.

Thank you.

[nicksavers]

@0xjac done, but could you please have @jbaylina confirm himself as the author? His reply above doesn't exactly make that clear. And could you also work on getting the status of EIP-820 to Superseded?

[0xjac]

@nicksavers Thank you for merging.

I send a message to @jbaylina and he should confirm himself he is an author soon. (He is traveling right now, but hopefully he should have 5min and internet at some point this weekend.)

Regarding the status of 820, it was my understanding that things had to happen in the following order:

1. Put this EIP to last call.
2. Have this EIP approved and set to final.
3. Set the status of 820 to superseded.

If you would like me to do things in a different order, that's fine. Just let me know.

[nicksavers]

That order seems to just fine to me.

[jbaylina]

@nicksavers I confirm that I'm the coauthor of ERC820 and ERC1820. And I'm working together with @0xjac .
ERC1820 is just ERC820 to adapted to Solidity 0.5

[0xjac]

Review from @mcdee:

I have checked ERC-1820 as a diff against ERC-820 and it looks good in terms of the changes.

I have read ERC-1820 and found no wayward instances of '820' where a search/replace might have gone wrong.

I have read and tested the ERC-1820 contract and it appears to function as expected. There is a small discrepancy in the comments for implementsERC165Interface() where it states that the function returns "True if _contract.address() implements _interfaceId, false otherwise." but the parameter _contract is already an address. This is not something that I consider significant enough to change the contract for at this stage, however, as the input is clearly labelled and there is no impact on functionality.

A minor note: created-by is incorrect; year should be 2019 not 2018.

[fulldecent]

I reviewed the 165 part, everything looks good. 👍

[lrgeoemtry]

How to add this to an already deployed token?

[0xjac]

@mcdee Regarding the _contract.address(), i was using natspec's dynamic expressions. But I am not a fan of it and I am ok to remove it. @jbaylina any thought?

The created-by has been fixed. (See #1865.)

@fulldecent Thank you for your review.

@lrgeoemtry What type of contract, 777? My suggestion at this time is to wait a couple weeks and wait for 777 to be finalized. If you want to deploy a token sooner, add a function which can change set the address of the registry once and call that function once ERC777 is final.

Quick, dirty and not audited example given without any guarantee:

MyToken is ERC777Token {
    ERC1820Registry registry;
    bool registrySet;

    setERC1820Registry(address _registry) {
        require(!registrySet);
        registry = ERC1820Registry(_registry);
        registrySet = true;
    }

    // ERC777 functions ...
}

[mcdee]

@0xjac if contract.address() is there for a purpose you could leave it in; as mentioned it doesn't materially alter the contract.

[0xjac]

@mcdee it is but

1. I cannot make it work properly
2. Natspec is not used that much
3. It's confusing

Therefore I have removed it. It trigger a change of address but there is no change to the code so previous reviews still apply.

• The EIP has been updated.
• The library has been updated to v0.0.2
• The new version has been deployed on mainnet and all test nets.

The address is 0x1820a4b7618bde71dce8cdc73aab6c95905fad24.

I am meeting with Jordi tomorrow to finish 777

[0xjac]

There was no significant changes and no functional changes to ERC1820.
The only change made was a change to the natspec comments on the registry.
(See 556cc58 for the changes.)

Thanks to @mcdee and @fulldecent for the reviews.

@nicksavers Could you please merge #1919 and make 1820 final? Thanks

[AC0DEM0NK3Y]

How is this standard to be used for local testing? Obviously there is the testnet deployments but compiling the registry contract locally yields a different contract address than the 0x1820 vanity via truffle or remix either forced to 0.5.3 or otherwise.

I suppose we should plug in the contract address during construction and store it rather than use a constant?

[mcdee]

@AC0DEM0NK3Y you could deploy the bytecode in the ERC directly as a truffle migration, as per https://github.com/wealdtech/wealdtech-solidity/blob/master/migrations/2_erc1820.js

[wighawag]

Hi, there is a bug in the current 1820 implementation due to the mechanism used to check for EIP-165 interfaces.

I posted the issue in detail on EIP-165 github issue here where the bug is also present in the example on how to read an ERC165 contract.

The basic problem for EIP-1820 is that when a contract call eip1820Registry.getInterfaceImplementer with an EIP-165 interface id it is possible that the call is giving less than 30,000 gas to supportsInterface .
If that is the case and the implementation of supportsInterface is using more than ~ 22,000 gas (see test case here) the call to supportsInterface throw, but the eip1820Registry.getInterfaceImplementer has enough to complete and return false.

The parent call conclude wrongly that the contract is not supporting the interface in question. If this was used as a last check and that enough gas is available to complete the call (EIP-150 give Math.floor(gas/64), such behaviour could have unwanted consequences.

While there might be no EIP-165 supportsInterface implementer that use more than 22000 gas (as this is an unlikely high use of gas), the fact that it is a possibility make EIP-1820 broken as per EIP-165 spec.

If we could make sure that no contract in existence implement supportsInterface with more than X gas, we could make a change in EIP-165 to lower the max gas, but this is quite dependent on how gas pricing might evolve.

One possible workaround would be for the caller of eip1820Registry.getInterfaceImplementer to ensure enough gas is provided to it (the exact value to be computed) as this would ensure in turn that supportsInterface is provided enough gas.

Also note that the cache should not be at risk though since it requires much more gas than was can be left as per EIP-150 rules. As such another workaround would for the caller of EIP-1820 to first update the cache.

In my opinion, it would be better to fix the issue rather than using such workaround.

There are few ways to properly fix it in the EIP-1820 implementation (see EIP-165's discussion ) but the best option is the solution proposed by #1930 itself. It would be great to get this in in the next hardfork, or at least get the conversation going since this is an issue affecting other use-cases like meta-transaction.

[jbaylina]

@wighawag I think that the easy way is to force in ERC-165 that the gas price must be under 20000 (Or some value that make sense). I would like to see the real use case where you need more than 20000 gas.

[wighawag]

yes, I am not sure why 30,000 was allowed in the first place. We should check if there is no existing contract out there that use that amount of gas though since changing ERC-165 that way would break them.

But note that while 20,000 might work for 1820 (a closer look is required), ERC165 checker implementation used elsewhere can have the problem with 10,000 gas (maybe lower)

[fulldecent]

Discussed from #881: Please provide a test case which shows this which we can paste into Remix IDE to study.

Discussed from #1155: I cannot defend the choice of 30,000 maximum gas on technical merits. This was settled very early on and I inherited it during my involvement in ERC-165.

[wighawag]

As mentioned there is a test case here : https://github.com/wighawag/ethereum_gas/blob/master/test/testERC1820.js
the ERC1820 caller that is vulnerable: https://github.com/wighawag/ethereum_gas/blob/master/src/Test1820Registry.sol

You can think of it this way:

• does destination implements X ?
• if not continue
• if yes call X and revert the call if it throw (maybe because X act as a mechanism to let the destination reject reception of tokens)

Because of the bug it is possible for

• destination to be set to return true for supportsInterface(X)
• but throw because call with less than 30,000 gas
• the previous logic interpret this as teh destination not implementing X and proceed with the call (this means the destination could not reject the tokens that it was going to if called with enough gas)

[fulldecent]

Thank you for the comprehensive references. There's a lot of code there. Which code do I need to run, preferably in Remix IDE, to minimally demonstrate the problem?

[wighawag]

for 1820 you can look at

• ERC165MoreGasExample that is the one implementing supportsInterface and require a amount of gas specified approximatively by the constructor
• Test1820Registry that is the one calling the registry

for 165:
you can reuse ERC165MoreGasExample
and use

• TestERC165StandardExampleViaCall for using ERC165Query as library contract
• TestERC165StandardExample for using ERC165Query as an base contract
• TestERC165 for using a variant of openzeppelin implementation

[honzens]

If It should change the following code
require(getManager(_addr) == msg.sender, "Not the manager");
to
require(getManager(_addr) == msg.sender || _addr == msg.sender, "Not the manager or owner");
I don't know why the owner can not revoke his Manager.

[jackromo888]

• [ ]