Initial migration to Solidity 0.6.x - v3.0 first steps

[nventuro]

This is a work in progress for a migration of the library to 0.6.x.

This migration is particularly problematic because of the addition of override. We rely on multiple-inheritance and the linearization of the inheritance graph to optionally extend behavior by inheriting from multiple variants (e.g. MyToken is ERC20Mintable, ERC20Pausable will combine both) that each override some base functions, often calling the parent version as well (via super).

In 0.6.x, this must be made explicit on the derived contract whenever there is diamond inheritance. Otherwise, an error message similar to the following will be displayed:

contracts/mocks/MyERC20.sol:6:1: TypeError: Derived contract must override function "_transfer". Two or more base classes define function with same name and parameter types.
contract MyERC20 is ERC20Pausable, ERC20Mintable {
^ (Relevant source part starts here and spans across multiple lines).
contracts/token/ERC20/ERC20.sol:241:5: Definition in "ERC20": 
    function _transfer(address from, address to, uint256 amount) internal virtual { }
    ^------------------------------------------------------------------------------------------^
contracts/token/ERC20/ERC20Pausable.sol:19:5: Definition in "ERC20Pausable": 
    function _transfer(address from, address to, uint256 amount) internal virtual override whenNotPaused {
    ^ (Relevant source part starts here and spans across multiple lines).

This will be a new burden placed on users of the library, and is sadly unavoidable (unless we were to provide every possible combination of variants, which wouldn't scale). To alleviate the situation, we decided to:

• Make the user-provided overrides as simple as possible: all that needs to be done is call super.foo()
• Add new, comprehensive extension points, minimizing the number of functions that need to be overridden by the user. Instead of _transfer, _mint and _burn, there'll be a single _afterTokensMoved function. This also came up as a need recently in Made private methods internal to allow for overriding #2027 and Made ERC777 registry and private functions internal. #2048.

What is currently implemented in this PR:

• Migrate all contracts to 0.6, except GSN, ERC721, ERC777 and Crowdsales
• Make all non-view functions virtual
• Add _afterTokensMoved and _afterTokensApproved ERC20 extension points, use those to implement variants (the same pattern would be repeated for 721 and 777)

[nventuro]

For reference, this is how creating an ERC20Mintable plus ERC20Pausable would look like:

pragma solidity ^0.6.0;

import "../token/ERC20/ERC20Mintable.sol";
import "../token/ERC20/ERC20Pausable.sol";

contract MyERC20 is ERC20Mintable, ERC20Pausable {
    function _afterTokensMoved(address from, address to, uint256 amount) internal override(ERC20, ERC20Pausable) {
        super._afterTokensMoved(from, to, amount);
    }

    function _afterTokensApproved(address from, address to, uint256 amount) internal override(ERC20, ERC20Pausable) {
        super._afterTokensApproved(from, to, amount);
    }
}

Note that the override keyword requires specifying which base implementations we're overriding: in this case, it is ERC20 and not ERC20Mintable because Mintable does not override any base implementation. While not obvious, the compiler error would guide users to doing the right thing:

contracts/mocks/MyERC20.sol:7:83: TypeError: Function needs to specify overridden contract "ERC20".
    function _afterTokensMoved(address from, address to, uint256 amount) internal override(ERC20Mintable, ERC20Pausable) {
                                                                                  ^------------------------------------^

[spalladino]

I like the decision of including the hooks, however there is one downside I see: most overrides actually add preconditions rather than side-effects. By adding the precondition check after the action occurs, the gas expenditure of a failed tx is made much higher. I'd consider adding both before and after hooks, and override the corresponding one as needed, even if this means that the end user will have to write twice the boilerplate in cases like the MyERC20 example. I'd also reconsider the tokenMoved name, as I'd rather go with tokenTransfer.

After this, I think the next step would be to decide which off-the-shelf combinations we want to ship for ERC20, ERC721, and ERC777. I'd consider the following to guide this decision:

• Which combinations or behaviours are the most popular
• Which combinations can be set up without forcing an opinionated access control solution
• When using an access control solution, which behaviours can be "shut down" using access control

[nventuro]

I like the decision of including the hooks, however there is one downside I see: most overrides actually add preconditions rather than side-effects. By adding the precondition check after the action occurs, the gas expenditure of a failed tx is made much higher. I'd consider adding both before and after hooks, and override the corresponding one as needed, even if this means that the end user will have to write twice the boilerplate in cases like the MyERC20 example.

For simplicity's sake I wanted to add a single hook (either 'before' or 'after'), and 'after' has the nice property of all values having been already validated (we could also use 'before' and keep this property, though it'd require more careful code - I'd rather we didn't).

Having both 'before' and 'after' could work: after all, once the user gets to our docs and figures out how to fix the issue, whether they need to fix one or four overrides will not be very important. There is one more reason to prefer less hooks however: the error messages are rather large, and four of them (two for each hook) will fill up a screen. C++ taught us this is less than ideal.

Do you know how many transactions actually revert on mainnet though? I often feel optimizing the gas cost of reverts is not very useful due to these transactions never being sent in the first place. try/catch and increased composition of systems with proper error handling may of course change this in the future.

[spalladino]

Hmm if I cannot convince you with the gas argument, let me pull out the big guns: forcing the dev to add the preconditions at the end breaks the check-effect-interaction security pattern. I wouldn't go against this, even if it implies larger compilation error msgs.

[nventuro]

Hm, I can get behind that. ERC20 doesn't really have an 'interaction' part, but it still makes sense to promote that pattern.

Alright, what about _beforeTransfer and _afterTransfer (from, to, amount), with the following properties:

• in both, a zero from address means tokens are being minted
• in both, a zero to address means tokens are being burnt
• from and to will never be both zero
• in before, it is possible the transfer is invalid for other reasons (such as from not having enough tokens)
• in after, all balances and state have already been updated

We'd extend the same general guiding principles to approvals.

[spalladino]

Works for me!

Do we want the _before hook to be able to modify anything from the transfer, such as the transfer value? My initial reaction is no - if a dev wants to change the behaviour of the transfer, they should override the method, not rely on the hook to do it.

[nventuro]

No, I wouldn't allow for that. The purpose behind hooks is to add more functionality (such as validations or extensions), not to alter the original one.

Your comment made me realize however that anyone who overrides _transfer will have to take care to remember to call the hooks - we should make a note about that.

[nventuro]

Looks like Solhint cannot handle 0.6.x code, so we won't be able to lint Solidity code until the new version comes out.

[nventuro]

It looks like solidity-docgen doesn't support 0.6 syntax yet, so the API reference module crashes: we won't get previews for that until we fix docgen: OpenZeppelin/solidity-docgen#133

[nventuro]

This should now be ready for an initial alpha release: the library is in a state similar to v2.4, albeit with Solidity v0.6 keywords (so explicit overrides are required). The main difference between v2.4 and this PR is that Crowdsales were removed: we decided not to port them due to a perceived lack of recent usage and technical difficulties in the migration to v0.6.

The final v3.0 release won't happen for at least a few weeks: we still need to iron out the details of the migration and work on new documentation. The main purpose behind this alpha release would be to enable early adopters that want to switch to v0.6 as soon as possible, hopefully gathering feedback during the stabilization process.

Regarding the hooks described above, I went ahead with _before and _after variants for ERC20 transfers and approvals, and I'm satisfied with the result. We still need to figure out what 'approval' means in the context of ERC721 and ERC777, but I think we can do without that for now.

[spalladino]

Great job! It seems the before/after hooks distinction was indeed a good idea.

[spalladino]

I hope the linter decides to put these 4 words on a single line

[nventuro]

I rebased on top of dev-v3.0 to keep the branch as sane as possible