Add ERC20 compatibility to ERC777.

[nventuro]

Fixes #1731.

Our ERC20 implementation has internal functions for _mint, _transfer, _burn and _approve, but I did not include any of these here to keep the API small for now. Note that we do have a _mint function, but it performs ERC777 minting (i.e. it requires a recipient contract to implement the token recipient interface).

Should we also add the functions to increase and decrease allowance?

[catageek]

Hi, I made a quick review of the contract, see my comments above. What do you think about implementing ERC20 compatibility as a switchable feature ? What if someone does not want ERC20 compatibility ?

[nventuro]

Thank you @frangio and @catageek for the review!

What do you think about implementing ERC20 compatibility as a switchable feature ? What if someone does not want ERC20 compatibility?

We thought about this and decided it'd be best to have support by default (which has the nice side effect of greatly simplifying the implementation). I'm not sure why someone would not want to have ERC20 support, but we can add a contract that removes it by having all of the ERC20 functions (transfer, approve, etc.) revert when called.

---

@frangio I'm considering getting rid of the granularity bit and hardcoding it to 1, which will both make the code simpler and cheaper by removing unnecessary requires, but also simplify the constructor. As of now, creating an ERC777 requires understanding what granularity is.

The spec says:

Most of the tokens SHOULD be fully partition-able. I.e., this function SHOULD return 1 unless there is a good reason for not allowing any fraction of the token.

WDYT?

[nventuro]

Hm, coverage decreased due to a require in approve that is correct, but can never be triggered by the contract as-is. approve used to be internal in ERC20, and was therefore more testeable. @frangio what do?

[frangio]

What do you think about implementing ERC20 compatibility as a switchable feature?

I'm considering getting rid of the granularity bit and hardcoding it to 1

In both of these cases I think for now we should target only the most common use cases. This means ERC20 support built-in, and granularity hardcoded to 1. However, I'd like to understand what a future API would look like if we ever decide to: 1) implement support for opting out of ERC20 compatibility, and 2) opting in to a different granularity.

[frangio]

Should we also add the functions to increase and decrease allowance?

I think so, yes. Why not?

[frangio]

@nventuro and I looked into the potential future API for opting out of ERC20 support and in to granularity.

We saw that we can put the ERC777 implementation alone in its own contract and, as long as we provide an internal _transfer function that does not call the recipient hook, it's possible to implement ERC20 compatibility on top of it with inheritance.

The same for granularity because it's just adding a couple require checks on internal functions.

[nventuro]

I've commented out the offending require statement, that should be it for this PR. I'll remove the granularity in a separate one, to make reverting that change easier once (if) we add opt-in support for it.

[frangio]

Just a thought... what do you think about adding a pending test (i.e. empty it) for the approve check that's missing coverage?

[nventuro]

Not sure what you mean, calling a public function on the mock that calls the non-existent internal function? What would be the point?

[frangio]

LGTM!

[frangio]

Looks good! Thank you @nventuro and @catageek!