ERC20 simplification with allowing _transfer() to perform _mint() and _burn() functionalities

[k06a]

Let's discuss this opportunity to minimize ERC20 source code.

PR Checklist

• [ ] Tests
• [ ] Documentation
• [ ] Changelog entry

[Amxx]

Hello @k06a

I'm really not confortable with this change. In particular, it would allow a dev to call _transfer() internally, and do "mint" or "burn" operation that do not update the totalSupply.

Can you give more details about your usecase ?

I was not here when the decision was taken, but having the mint & burn be separate function from transfer is helping with multiple things:

• its easy to override mint and burn, and that doesn't add any check in transfer, making sure no additional cost is introduced into the simple transfers (that are the most commonly used operations)
• it forces the dev to clearly distinguish between these operations and avoid the risk of minting or burning "by mistake"

[k06a]

@Amxx I see, but this issue could be solved by moving totalSupply modification into the _transfer method. Your points makes sense for me, but what about extracting such a private __transfer method? This would reduce the code size (+compiled size).

[Amxx]

Having an additional __transfer function would just make everything more confusing. Also, I think we should avoid __ préfixes.

What about leaving everything as now, and adding a _arbitraryTransfer that would redirect to mint, burn or transfer depending on the parameters it receives?

[k06a]

@Amxx this would work and can be introduced as an extension. But I was thinking about reducing code size of the existing solution.

[Amxx]

I'm more concerned by the gas cost of executing transactions then by the cost of deployment.

Maybe we should produce number to check the impact of such a change on both

[k06a]

I agree that both gas impacts are important.

[frangio]

I'd like to see the numbers but I believe the bytecode size impact could be significant, and the runtime cost impact may be negligible. The additional cost contains no storage reads or writes as far as I can tell, it's just stack operations and jumps.

I think we have to consider this improvement, and a similar strategy could be valuable in other contracts.

_arbitraryTransfer could be a private function if we prefer to keep the current internal API, which is simpler as it has a single way to do each operation.

[frangio]

We are probably going to move forward with this in 5.0 along with the removal of before and after hooks (replaced with "simple" overriding of _transfer). A PR with these changes for ERC721 in #3635.

[frangio]

This has been implemented in next-v5.0 in #3838 and #3921.