Causes problems with Diamond facets

[cliffhall]

Briefly, the Diamond Multi-Facet Proxy is a fine-grained proxy. It allows function selectors from multiple implementation contracts to be invoked with delegatecall rather than sending all unimplemented calls to a single implementation contract.

This pattern leans heavily on keeping track of which function selectors go with which contract. The developer must take care to make certain that proxied function selectors are unique across all facet contracts.

When adding a facet to a Diamond proxy, the developer passes in a list of function selectors that should be proxied by the given implementation contract. On the JS side, when deploying, we just get the function selectors from the deployed facet and remove any selectors we don't want proxied. For instance, supportsInterface(bytes4) is one that is often implemented by several contracts in a system, but when you put them all behind a Diamond, you want to be certain which one is going to answer that call, and ignore the existence of all the others.

So that works fine and well, except for when it comes to running tests with solidity-coverage. Apparently it is adding some instrumentation selectors that are present on all the facets. Since our unit tests don't take these unexpected function selectors into account, suddenly the unit tests dealing with adding new facets breaks because we're trying to add the same selector for multiple facets.

Clearly the workaround at the moment will be for us to do a little diagnostic display and figure out which selectors are added by solidity-coverage, and remove them from the list we're about to try and add to the Diamond. That will work for me, but as Diamond adoption grows, when multiplied across all the folks who'll be deploying this advanced contract architecture and seeking 100% confidence in it, seems... heavy.

I'd don't have a ready solution to offer here. A first help might just be documentation that enumerates all the function selectors that are added during instrumentation. Maybe next would be some function we could call with a contract to get those instrumentation selectors handed back to us in an array. We'd only want to do this is coverage is running, the function would need to be aware of that.

[mudgen]

Multiple people have reported problems using coverage with diamonds. If diamond support or tooling were added to/for solidity-coverage it would be used by people.

[cgewecke]

Thanks so much for the explanation and the suggestions. I still need to look into this a bit to really understand what's going on.

(Possible this should be grouped with #651)

[cgewecke]

@cliffhall

If you have a chance, could you see if the change in PR #660 resolves your test failures? (It restricts the instrumentation fns to internal visibility as long as they are contract (rather than file) scoped).

Have published the branch at tag eip165 so ...

yarn add solidity-coverage@eip165 --dev

[cliffhall]

Wooohooo!!! @cgewecke that fixed it! Thanks so much!

[cgewecke]

Okay awesome, thank you!

[failfmi]

Hey, is there a fix available in any of the solidity-coverage versions? cc @cgewecke

[cgewecke]

@failfmi

Yes, just published this (literally) 30 seconds ago: 0.7.18

Let me know if it doesn't work.

[failfmi]

@cgewecke, literally speechless. You rock, sir!

[cgewecke]

0.7.18