Migrate Contracts to Solidity v0.6

[nventuro]

Closes #2028

This a continuation of #2063, now bringing the dev-v3.0 branch into master, and the foundation of the v3.0 release. This merge, with perhaps some light edits, should become the v3.0-alpha release.

For more information about the v3.0 release, head to our roadmap.

This PR is quite massive, with a lot of noise. Below is a summary of the included changes:

• All contracts were migrated to ^0.6.0
• All non-view functions were made virtual
• _before hooks were added to ERC20, ERC721 and ERC777 to better support custom extensions, and to prevent users from having to manually override too many functions on diamond inheritance patterns
• Crowdsales were removed
• All Roles contracts were removed (except the Roles library). Contracts with access control baked-in were either removed (ERC20Mintable), or reduced to internal functions (Pausable)
• Tests were adapted were needed. This was mostly seamless except in the case of ERC721, were many of the tests shared code with ERC721Mintable

A couple undesirable side-effects product of the migration are:

• ERC20Migrator was removed (it relied on ERC20Mintable)
• ERC20's balanceOf and allowance are virtual despite being view. This is required for GSNRecipientERC20Fee companion token contract

I'd tackle these in separate PRs (if at all).

Note that these are not all the changes we want to include in v3.0. We're redesigning our Access Control solution and plan to have it implemented for the final release, and will also make some small changes such as removal of deprecated functions.

[nventuro]

We won't have a test coverage report until sc-forks/solidity-coverage#464 is fixed, sadly.

[spalladino]

@nventuro quite a large PR! Looks good (as good as an inspection over 201 changed files can go in a single sitting), but I have a comment about the beforeApproval hooks.

Right now, they are being called in two very different scenarios: 1) when the owner of the asset manually sets an approval, and 2) when it is decremented due to an action (such as a transferFrom). And it is not possible to determine, from within the hook, which case it is.

I checked the uses of beforeApproval within the library, and found only pausable. For this, only the first usage of the hook is actually needed, since the second one is covered by the beforeTransfer hook which also checks for the paused state. Thinking a bit more, if one wanted to implement a pause all approvals behaviour, then it would make sense to only pause 1), and not 2) - which is not possible to do today.

So, the conclusion of this rambling is: shouldn't we rename beforeApproval to beforeSetApproval, and only call it in the methods where approval is actually changed?

[nventuro]

Thanks for the review! You raise an interesting point, we should definitely be clear about what hooks mean.

Keep in mind Contracts features internal functions like _approve, which can be called arbitrarily by the contract (imagine some sort of signup during which a contract gives itself infinite allowance over the user's tokens). This means approve and transferFrom are not the only possible triggers for allowance changes.

The current hook behavior is quite generic and allows to react to all changes to the allowance, which is IMO a more powerful primitive: if what you want is to prevent users from calling approve, then the natural place to do it is at the approve external function.

[spalladino]

if what you want is to prevent users from calling approve, then the natural place to do it is at the approve external function.

What do you think about removing the `beforeApprove` hooks then, and implement pausable like this?

…

On Fri, Feb 7, 2020 at 5:34 PM Nicolás Venturo ***@***.***> wrote: Thanks for the review! You raise an interesting point, we should definitely be clear about what hooks mean. Keep in mind Contracts features internal functions like _approve, which can be called arbitrarily by the contract (imagine some sort of signup during which a contract gives itself infinite allowance over the user's tokens). This means approve and transferFrom are *not* the only possible triggers for allowance changes. The current hook behavior is quite generic and allows to react to *all changes to the allowance*, which is IMO a more powerful primitive: if what you want is to prevent users from calling approve, then the natural place to do it is at the approve external function. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2080?email_source=notifications&email_token=AADI4JG4GMRXR5IFTPZBC3LRBXAWTA5CNFSM4KRCDBV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELEQGIY#issuecomment-583598883>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADI4JGKUN74CXPSD2M5AYLRBXAWTANCNFSM4KRCDBVQ> .

[nventuro]

What do you think about removing the beforeApprove hooks then, and
implement pausable like this?

I'm actually considering removing ERC20Pausable altogether, leaving just the basic Pausable mechanism, along with an example ERC20 ('batteries-included') that showcases usage of Pausable. The v2.x version is opinionated in non-obvious ways: transfer is paused but _transfer and _mint aren't.

That leaves the question of which hooks we should provide. _beforeTransfer is clearly useful, but _beforeApproval less so: I wouldn't be against removing it, but I also don't want to sacrifice flexibility for the sake of minimalism.

[frangio]

an example ERC20 ('batteries-included')

We should be clear the batteries-included contracts are not meant as examples only. They are meant to be used.

[frangio]

Thanks for doing this work!

[spalladino]

That leaves the question of which hooks we should provide. _beforeTransfer is clearly useful, but _beforeApproval less so: I wouldn't be against removing it, but I also don't want to sacrifice flexibility for the sake of minimalism.

Keep in mind all methods are still virtual. We are not sacrificing flexibility, the user can still implement this - only that they will have to explicitly choose which approval changes they want to react upon. And yes, deal with the Solidity 0.6 overrides() complexity.

[nventuro]

Keep in mind all methods are still virtual. We are not sacrificing flexibility, the user can still implement this

Hm, you're right, users can always do e.g.

function _approve(...) override {
    _beforeApproval();
   super._approve();
   _afterApproval();
}

It wouldn't quite be the same since not all scenarios may be covered (e.g. _mint changes balances but doesn't call _transfer, which the hooks account for), but given this and that we can always add them later, I'm fine with removing the approve-related hooks.

[frangio]

I think I'm with @spalladino on this one. I think we may want to review whether it's a good idea that transferFrom uses _approve internally. In the meantime, removing the hook sounds like a reasonable compromise.

[nventuro]

Thanks for the reviews! I've updated the token contracts to remove the allowance and operator hooks, removing also the associated functions from the Pausable token variants for simplicity.

We'll want to think some more about the conditions that can trigger a hook, and which ones we want to provide initially.

[nventuro]

Thanks @frangio and @spalladino for the reviews!

[alexanderatallah]

@nventuro similar to having ERC20's allowance be non-virtual, can we have ERC721/1155's isApprovedForAll be non-virtual? This is needed for developers who want to whitelist some contract for approval (maybe one that they're building, or in our case, the OpenSea proxy for a user)

[frangio]

@alexanderatallah Did you mean virtual (as opposed to non-virtual)?