Assumed decimals must be 18

[vittominacori]

You are assuming here that decimals must be 18.

Also if this is true for most of the token and I always use 18, there are lot of tokens using 2, 6, 8 or other numbers.

How much is token deployment cheaper having decimals not stored? And is it so important to reduce gas in reading slot on a view method like decimals()?
At this point, why we are not reducing gas on name and symbol the same way we are doing for decimals?

I think a library should be more extensible and not take decisions on business logic.

In v4.0.0 there is the try to "save" some point of gas by introducing complexity for end users or breaking some first days logic (like the impossibility to check cap during construction #2580).

[abcoathup]

Hi @vittominacori,

Thanks for the feedback, it is really appreciated.

We can override decimals() to specify a decimals other than 18. See: https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20-decimals--

The project owner will review your feedback as soon as they can.

Please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don’t want you to waste your time!

[vittominacori]

Yes I know that we can override but it is introducing lot of stuff.

If I would like to set decimals in construction basically I should replicate the old _setupDecimals behavior.

Also if you override this method, any other derived contract must override too in order to avoid TypeError: Derived contract must override function "decimals". Two or more base classes define function with same name and parameter types.

[frangio]

Hi @vittominacori, your feedback is appreciated. I'll reply to each of your points separately.

You are assuming here that decimals must be 18.

Also if this is true for most of the token and I always use 18, there are lot of tokens using 2, 6, 8 or other numbers.

What we're assuming is that 18 decimals is the most common value. We're not ignoring that someone might want to change the decimals value, but the way to do this is now to override the function.

How much is token deployment cheaper having decimals not stored? And is it so important to reduce gas in reading slot on a view method like decimals()?

Around 20k gas cheaper (22k with Berlin hardfork changes). This is around 1% of deployment cost. It also reduces bytecode size and overall deployment cost, but I don't have the exact numbers on this.

At this point, why we are not reducing gas on name and symbol the same way we are doing for decimals?

Because there is no standard value for name and symbol like there is for decimals. Everyone would have to customize those values through inheritance, which we recognize is not comfortable, so constructor parameters are still preferred. It's acceptable for decimals because customizing that is an exception.

Also if you override this method, any other derived contract must override too

Can you give an example about this? What I can think of is that if you define a contract ERC20WithDecimals and then define another contract Derived is ERC20, ERC20WithDecimals you will have to override to resolve the multiple inherited definitions for decimals. Is this what you were thinking?

[vittominacori]

Hello thanks for your reply.

Yes, I mean that having that contract

abstract contract ERC20WithDecimals is ERC20 {
    uint8 immutable private _decimals;

    constructor (uint8 decimals_) {
        _decimals = decimals_;
    }

    function decimals() public view virtual override returns (uint8) {
        return _decimals;
    }
}

and the derived one

contract DerivedERC20 is ERC20, ERC20WithDecimals {

    constructor (
        string memory name_,
        string memory symbol_,
        uint8 decimals_
    )
        ERC20(name_, symbol_)
        ERC20WithDecimals(decimals_)
    {}
}

we will have the following error

TypeError: Derived contract must override function "decimals". Two or more base classes define function with same name and parameter types.
 --> contracts/DerivedERC20.sol:5:1:
  |
5 | contract DerivedERC20 is ERC20, ERC20WithDecimals {
  | ^ (Relevant source part starts here and spans across multiple lines).
Note: Definition in "ERC20": 
  --> @openzeppelin/contracts/token/ERC20/ERC20.sol:84:5:
   |
84 |     function decimals() public view virtual returns (uint8) {
   |     ^ (Relevant source part starts here and spans across multiple lines).
Note: Definition in "ERC20WithDecimals": 
  --> contracts/ERC20WithDecimals.sol:12:5:
   |
12 |     function decimals() public view virtual override returns (uint8) {
   |     ^ (Relevant source part starts here and spans across multiple lines).


Error HH600: Compilation failed

[frangio]

In this particular case, you can make the following change to get rid of that error:

-contract DerivedERC20 is ERC20, ERC20WithDecimals {
+contract DerivedERC20 is ERC20WithDecimals {

This fix might not work with more complex inheritance lists.

[vittominacori]

Yes it was an example taken from your reply :)

It doesn't solve if I have something like this.

contract DerivedERC20 is ERC20Burnable, ERC20WithDecimals {

    constructor (
        string memory name_,
        string memory symbol_,
        uint8 decimals_
    )
        ERC20(name_, symbol_)
        ERC20WithDecimals(decimals_)
    {}
}

[frangio]

I'm going to close this, as it was a design decision we made based on the fact that the vast majority of ERC20 tokens have 18 decimals.

We recognize that there is now more friction to define a different decimals number, but it is definitely still possible to do so.