Should `skipSupportsInterface` be `true` for Diamond facets?

[RogerPodacter]

This issue was raised in https://github.com/wighawag/hardhat-deploy/pull/216 and marked solved, but I still get the error:

SkipSupportsInterfaceTest.sol:

pragma solidity 0.8.17;

contract SkipSupportsInterfaceTest {
    function supportsInterface(bytes4 _interfaceId) external pure returns (bool res) {
    }
}

Deployment script:

module.exports = async ({ getNamedAccounts, deployments }) => {
  const { diamond } = deployments;
  const { deployer } = await getNamedAccounts();

  await diamond.deploy("TestDiamond", {
    from: deployer,
    log: true,
    waitConfirmations: 1,
    facets: ["SkipSupportsInterfaceTest"],
  });
}

When I deploy I get this error: Error: function "supportsInterface" will shadow "supportsInterface". Please update code to avoid conflict.

I believe this would go away if skipSupportsInterface were set to true, and there would be no downside as supportsInterface isn't appropriate on facets anyway.

Thanks!

[wighawag]

You ll have to provide your own Diamond contract as the one used by default as a builtin facet for supportsInterface

[RogerPodacter]

This actually was an issue not with your Diamond contract, which I believe handles this correctly, but rather with adding other people's base contracts that also define supportsInterface.

E.g., most ERC721 base contracts define it (even those that use Diamond Storage).

And then if you want ERC2981, the base contract you use probably defines supportsInterface as well, and so now you've got three definitions!

To resolve this I copy-pasted these base contracts into my code base and removed supportsInterface, leaving hardhat-deploy's contracts unchanged.

Not a super-clean solution, but I cannot think of another way as it is such a strong convention for base contracts to provide the correct supportsInterface!

However this issue might be good to mention in the README. Just a thought!

[wighawag]

Ha ok, yes, one plan is to have a filter option to let you define what function should be exposed for each facet That would solve your problem too. Thanks for clarifying

[RogerPodacter]

That would be amazing!!

This would also solve another infuriating scenario: I have a contract with 10 functions and one function has a bug. Currently I have to fix the bug, deploy a new contract with the fix, and then diamond cut away all of the old functions and add the new ones!

Ideally I would deploy a new contract with just one single fixed function and then put the buggy function on the "filter" list of the old contract, meaning that 9/10 functions of the old contract make it and 1/1 functions in the new contract do.

Thoughts? This is my true dream!

[idkravitz]

That would be amazing!!

This would also solve another infuriating scenario: I have a contract with 10 functions and one function has a bug. Currently I have to fix the bug, deploy a new contract with the fix, and then diamond cut away all of the old functions and add the new ones!

Ideally I would deploy a new contract with just one single fixed function and then put the buggy function on the "filter" list of the old contract, meaning that 9/10 functions of the old contract make it and 1/1 functions in the new contract do.

Thoughts? This is my true dream!

I added a PR, that allows filtering, that would work in your scenario like patching a single function