Don't rename interfaces IWhatever -> IWhateverUpgradeable

[gnarvaja]

Also reported some time ago in https://forum.openzeppelin.com/t/why-contracts-upgradeable-has-ierc721upgradeable-and-i-whatever-upgradeable/14793

💻 Environment

openzeppelin-contracts == 4.3.2
openzeppelin-contracts-upgradeable == 4.3.2

📝 Details

The package openzeppelin-contracts-upgradeable renames the interfaces when they are exactly the same as non-upgradeable (vanilla) versions. This isn't necessary and brings a lot of issues like this one

🔢 Code to reproduce bug

interface IMyContract is IERC721 {
...
}

contract MyContract is IMyContract, ERC721Upgradeable {
...
}

Then when I compile I get errors like these:

11 | contract PolicyNFT is IERC721,
   | ^ (Relevant source part starts here and spans across multiple lines).
Note: Definition in "ERC721Upgradeable": 
   --> @openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol:184:5:
    |
184 |     function safeTransferFrom(
    |     ^ (Relevant source part starts here and spans across multiple lines).
Note: Definition in "IERC721": 
   --> @openzeppelin/contracts/token/ERC721/IERC721.sol:136:5:
    |
136 |     function safeTransferFrom(
    |     ^ (Relevant source part starts here and spans across multiple lines).

TypeError: Derived contract must override function "safeTransferFrom". Two or more base classes define function with same name and parameter types.

I think XXXUpgradeable contracts shouldn't define new interfaces but instead implement the standard ones. Interfaces are the same between upgradeable and non-upgradeable contracts.

If name clashes are an issue when importing files from contracts and contracts-upgradeable, those can be easily avoided using explicit imports

import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

[gnarvaja]

Related to this issue, OpenZeppelin/openzeppelin-upgrades#455, it's impossible to switch from SafeERC20 to using SafeERC20Upgradeable unless you change your variables from IERC20 to IERC20Upgradeable.

Changing this issue with the names of the interfaces will make contracts-upgradeable much more usable.

[frangio]

Thanks for reaching out about this. We're not going to make this breaking change in the near future but we'll consider it for the next major release.

[frangio]

There are no updates yet beyond my last comment but I want to provide some more context for why this was done this way initially.

Ideally this package (@openzeppelin/contracts-upgradeable) would depend on @openzeppelin/contracts and simply import the interfaces that are already defined there. However, transitive dependencies in Solidity libraries are not common, it is unclear how the various tools (Truffle, Hardhat, etc.) would deal with them, and we should not expect it to work the same across them or even work at all. The reason for this is that these tools resolve dependencies from node_modules but likely do not implement the full Node resolution algorithm, which resolves relative to the file that has the import. This means that npm dependencies can't be used because they assume imported dependencies will be resolved according to Node's resolution algorithm. Note that, in fact, because of the way the Solidity compiler has defined its API, it is not possible to resolve transitive dependencies exactly like Node does in the general case. Regardless, it is arguable undesirable to use the same algorithm as Node because it can result in a mess of conflicting dependency versions installed alongside.

The only possible alternative to avoid all the mess I just described is to use npm peer dependencies. Until npm v7 these were not great because they were not installed automatically, so they resulted in a poor user experience.

Because we chose not to use dependencies, these interfaces need to be defined within the package, and we decided to apply the same rename (*Upgradeable) in order to avoid conflicts with the non-renamed versions in case they were used side by side.

Now that peer dependencies have better usability, I think the next version of this package will declare OpenZeppelin Contracts as a peer dependency and import the interfaces from there.

[frangio]

This needs a few changes in the transpiler to get fixed. See OpenZeppelin/openzeppelin-transpiler#108.