search

sc-forks / solidity-coverage

Code coverage for Solidity smart-contracts
MIT License
977 stars 264 forks source link

Injected function modify type(XXX).interfaceId #651

Closed Amxx closed 2 years ago

Amxx commented 3 years ago

ERC165 enables some form of contract introspection, by advertizing supported interfaces. Some implementation hardcode the interface identifier

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
    return interfaceId == 0x12345678 || super.supportsInterface(interfaceId);
}

And others use solidity's built-in interfaceId getter

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
    return interfaceId == type(SomeInterfaceOrContract).interfaceId || super.supportsInterface(interfaceId);
}

IMO, these mechanism is to be tested in unit tests, to achieve full coverage of the code. However, we noticed that these tests do fail when SomeInterfaceOrContract is an abstract contract, possibly because functions are inserted by the coverage tool. This causes the corresponding tests to fail under coverage.

How can we prevent that? The abstract contract I'm talking about does not contain any storage definition, and all function definitions are abstract public or external functions.

cgewecke commented 3 years ago

Possibly similar issue in #649 (Diamond) ...

cliffhall commented 3 years ago

Have you tried making a test-only contract that extends the abstract contract and run your tests against it?

Amxx commented 3 years ago

I'm not sure what you mean.

We have our abstract contract, we have implementations, and we run our tests against the implementation. What would a test-only contract be ?

cliffhall commented 3 years ago

What would a test-only contract be ?

Err... a contract meant only for testing. Reading your issue description, it sounded like you were trying to test the abstract contract.

cgewecke commented 3 years ago

@amxx

If you have a chance, could you see if the change in PR #660 resolves your test failures?

Have published the branch at tag eip165 - you can install with: 

yarn add solidity-coverage@eip165 --dev
cgewecke commented 2 years ago

This should be fixed in 0.7.18. Please just ping if you're still having problems getting these tests to pass.