ERC20Snapshot should use hooks

[nventuro]

As of v3.1-rc.0, ERC20Snapshot is implemented by overriding _transfer, _mint, and _burn. This is problematic because it causes multiple inheritance issues for users that extend from both Snapshot and any other ERC20-based contract (e.g. ERC20Burnable).

We should use the hooks mechanism instead to implement this:

function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override {
  super._beforeTokenTransfer(from, to, amount);

   if (from == address(0) || to == address(0)) {
        // mint or burn
        _updateAccountSnapshot(from);
        _updateTotalSupplySnapshot();
  } else {
        // transfer 
        _updateAccountSnapshot(from);
        _updateAccountSnapshot(to); 
   }
}

It's not clear to me to what extent this would be considered API breakage. @frangio thoughts?

[frangio]

Technically this is a breaking change. Realistically, I don't think we can commit to never adding new overrides. I think that justifies adding the new _beforeTokenTransfer override. Once we do that, people will already have to change their derived contracts, and we might as well go ahead and remove the existing overrides.

I don't think this qualifies as a bug really.

[nventuro]

Heh sorry, I missclicked on the bug tag.

I think your reasoning is sound. We should probably add a section to the hooks docs where we explain what to do if there are conflicting definitions for the hook and the user must manually override them, including the expected error message so hopefully googling for it takes people there.

[frangio]

Yes, we do have the "Using Hooks" section but including the error message would be very useful for searchability.

[nventuro]

I meant that that guide tells you how to write hooks, but not what to do in the case of having to provide an override due to multiple inheritance.

[julianmrodri]

@nventuro @frangio Im in for this if you are OK. Just let me know thanks!

[julianmrodri]

@nventuro @frangio took a shot on the functionality. Let me know how it looks, I will take a look at the Docs change as well.

@nventuro when you talk about explaining what to do with confilcts with multiple inheritance you mean doing something like this?

contract MyERC20 is ERC20Snapshot, ERC20Burnable { 
  ....
  function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override(ERC20Snapshot, ERC20Burnable) {
    ERC20Snapshot._beforeTokenTransfer( from, to, amount);
  }
  ....
}

[frangio]

when you talk explaining what to do with confilcts with multiple inheritance you mean doing something like this?

More or less, except our recommendation is to use super._beforeTokenTransfer rather than the explicit parent name.