Remove override for functions from interfaces & further improvement suggestions (for v5.0)

[pcaversaccio]

🧐 Motivation

All the base implementation contracts use currently the override modifiers since Solidity requires the override keyword when implementing a function from a parent interface. @frangio once opened an issue here. A simple example:

function totalSupply() public view virtual override returns (uint256) {
    return _totalSupply;
}

Since Solidity 0.8.8 a function that overrides only a single interface function does not require the override modifier anymore. I wanted to quickly discuss what kind of Solidity version is planned for v5 contracts as well as such a refactoring would be plausible if upgraded to solc >=0.8.8.

[frangio]

what kind of Solidity version is planned for v5

0.8.8 is totally acceptable. I would be willing to go to a very recent version if it gets us good language features.

[pcaversaccio]

If this is the case, let me push that idea further - what about 0.8.18 (the latest version)? The reason why I would love to consider this version explicitly, it allows named parameters in mapping types which would increase readability imho. OZ contracts make extensive use of mappings, and this could be definitely a benefit:

mapping(address owner => mapping(address spender => uint256 amount)) private _allowances;

[0xad1onchain]

How about introducing type-safe bytecode encoding introduced in 0.8.11?

SafeERC20.sol builds bytecode via abi.encodeWithSelector

    function safeTransfer(IERC20 token, address to, uint256 value) internal {
        _callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
    }

Would propose changing this to abi.encodeCall which adds type checks for arguments passed for bytecode generation and would fail compilation if incorrect

   function safeTransfer(IERC20 token, address to, uint256 value) internal {
       _callOptionalReturn(token, abi.encodeCall(token.transfer, (to, value)));
   }

[balajipachai]

How about introducing type-safe bytecode encoding introduced in 0.8.11?

SafeERC20.sol builds bytecode via abi.encodeWithSelector

    function safeTransfer(IERC20 token, address to, uint256 value) internal {
        _callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
    }

Would propose changing this to abi.encodeCall which adds type checks for arguments passed for bytecode generation and would fail compilation if incorrect

   function safeTransfer(IERC20 token, address to, uint256 value) internal {
       _callOptionalReturn(token, abi.encodeCall(token.transfer, (to, value)));
   }

This is addressed in PR #4293

[balajipachai]

@frangio Any guidelines on how to proceed on this or is it been taken care internally?

[frangio]

A PR is welcome. Our only concern is that for a review we need to know how to check that the change was done exhaustively.

[balajipachai]

A PR is welcome. Our only concern is that for a review we need to know how to check that the change was done exhaustively.

Should we also consider named parameters for mapping types?

[pcaversaccio]

Since we extended the scope of this issue, I amended the title for clarity fyi.

[RenanSouza2]

A PR is welcome. Our only concern is that for a review we need to know how to check that the change was done exhaustively.

I removed every override and reinstated the ones needed to compile and those in comments. I'm quite sure it was exhaustively but more verificications are never enought

[pcaversaccio]

#4059 (comment) @ernestognw @frangio is this somewhere tracked as a separate issue so we don't forget about?

[frangio]

@pcaversaccio Feel free to open an issue.

[RenanSouza2]

@pcaversaccio If I may suggest a format, a metaissue with a list of all the pattern updates and then an issue per item so it is easier to track and discuss

[pcaversaccio]

@pcaversaccio If I may suggest a format, a metaissue with a list of all the pattern updates and then an issue per item so it is easier to track and discuss

I think that would make sense, however, that meta-issue should be managed by the OZ team imo. In any case, I opened an issue on the named parameters in mapping types here #4343.