Refactor ERC20Facet

[mudgen]

We need to start refactoring our existing facets to match the new design documentation of Compose.

The Permit functionality in the existing ERC20Facet needs to be pulled out of that and put in its own facet that composes with the ERC20Facet.

The burn and burnFrom functions need to be pulled out of ERC20Facet and put in its own facet that composes with the ERC20Facet.

The order of the variables in ERC20Storage struct should change so that the variables not used by extensions should be placed last so they are removed in extensions. This is according to the Extending Facets documentation: https://compose.diamonds/docs/design/design-for-composition#extending-facets

Helpful Information

Have a question? Please check our contribution guide first - your answer might already be there!

Want to discuss something? For general questions, ideas, or brainstorming, please browse our discussions or start a new one.

You can also join our Discord to discuss the issue.

[akronim26]

Can I take this up?

[mudgen]

@akronim26 Absolutely, it is yours.

[mudgen]

@akronim26

I think the ERC20Storage struct should be changed to this order:

 struct ERC20Storage {        
        mapping(address owner => uint256 balance) balanceOf;        
        mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
        uint256 totalSupply;
        string name;
        string symbol;
        uint8 decimals;       
    }

Because I think balanceOf and allowances and totalSupply will most often be used by extensions of ERC20, and the other variables can be removed from the struct in those facet extensions that don't use them.

[akronim26]

@mudgen I wanted to clarify something regarding ERC20Storage. In the new facet I’m creating for the permit function, nonces is required to prevent the replay attacks while permitting. Because of the same reason, it seems the previous version of the struct would be better. Could you confirm this version of the struct?

struct ERC20Storage {
        string name;
        string symbol;
        uint8 decimals;
        uint256 totalSupply;
        mapping(address owner => uint256 balance) balanceOf;
        mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
        mapping(address owner => uint256) nonces;
    }

[mudgen]

@akronim26 Thanks for clarifying this.

The nonces mapping should be removed out of the ERC20Storage struct, because the ERC20Facet does not need it.

The Extending Facets section of the documentation says this:

A facet that adds new storage variables must define its own diamond storage struct.

So the new ERC20PermitFacet needs to define its own diamond storage for the nonce mapping.

[akronim26]

Okk, thanks for clarifying, @mudgen!