ERC20Capped : do not make _cap private

[SvenMeyer]

I would like to create a token which has 2% inflation per year, based on ERC20Capped
I would still use the _cap which is initialzed in ERC20Capped but would then override cap()
Surprisingly, i can not access _cap to do the math.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

// import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Capped.sol";
import "@openzeppelin/contracts/security/Pausable.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";



contract COIN is ERC20Capped, AccessControl {
    bytes32 public constant MINTER_ROLE   = keccak256("MINTER_ROLE");

    uint256 public constant SECONDS_PER_YEAR = 36525 days / 100;
    uint256 public constant INFLATION = 2000; // 2.000 %
    uint256 public immutable DEPLOYMENT_TIME;

    constructor() ERC20("inflationary COIN", "COIN") ERC20Capped(100000000) {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
        DEPLOYMENT_TIME = block.timestamp;
    }

    /**
     * @dev Returns the cap on the token's total supply
     * @dev ERROR : _cap not accessible here ! - TODO
     */
    function cap() public view override returns (uint256) {
        return _cap + _cap * (block.timestamp - DEPLOYMENT_TIME) * INFLATION / SECONDS_PER_YEAR / 100 / 1000;
    }

    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
        _mint(to, amount);
    }
}

[frangio]

See "All state variables should be private" in GUIDELINES.md.

You can use super.cap() or you can copy-paste the contract and modify it since it's very simple.

[SvenMeyer]

@frangio As I prefer to make the upgrade to v5 as easy as possible, I went with your suggestion to use super.cap() .. thanks for that !

    import { ERC20, ERC20Capped } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Capped.sol";
~~~~~~~~~~
    function capAt(uint256 t) public view returns (uint256) {
        uint256 initialCap = super.cap();
        if (t < DEPLOYMENT_TIME) return 0;
        return initialCap + ((initialCap * (t - DEPLOYMENT_TIME) * INFLATION) / SECONDS_PER_YEAR / 100 / 1000);
    }

    /**
     * @dev Returns the cap on the token's total supply
     */
    function cap() public view override returns (uint256) {
        return capAt(block.timestamp);
    }

[barakman]

You are losing quite a bit of precision here; you may as well do something like:

return initialCap * ((t - DEPLOYMENT_TIME) * INFLATION + FACTOR) / FACTOR;

Where the value of FACTOR is SECONDS_PER_YEAR * 100 * 1000.

The general idea is to rearrange the calculation such that:

1. It includes a single division operation
2. That division operation is the very last operation to take place

The only thing which typically prevents applying this solution is an arithmetic overflow which may potentially occur during the computation (i.e., as a result of the intermediate value becoming larger than 256 bits).

But your case doesn't seem to fall under this category (unless the initial cap is extremely large, in which case, an arithmetic overflow is likely to occur anyway).

So in short, it seems sufficiently safe to do it this way.

[barakman]

BTW, it is not really clear why you use both 100 and 1000 in / 100 / 1000.

You should be using only the value which represents the resolution of INFLATION.

For example:

• If INFLATION = 25 denotes 25%, then you should use 100
• If INFLATION = 25 denotes 2.5%, then you should use 1000

[SvenMeyer]

@barakman Thanks for the suggestions ... I was actually aware of the precision problem, thus I do all the multiplications first and then divide at the end. I was thinking about doing it as you suggested, but thought (think) it does not make any difference as the compiler would do it, in my case it would not make any difference, and it would be more readable ... 🤔

As we are dealing with integer math, the 1000 is just to allow fractional percentage with 3 fractional digits , like 1234/1000 = 1.234% , while the 100 does the "percentage thing" 2% = 2/100
I was hoping if I write it that way, that it would be clear(er)

[SvenMeyer]

Maybe a good idea anyway ... ;-)

uint256 public constant INFLATION = 2000; // 2.000 %
~~~~~~~~~~~~~~~~~~~~~~~~~~
return initialCap + ((initialCap * INFLATION * (t - DEPLOYMENT_TIME)) / (SECONDS_PER_YEAR * 100 * 1000));

[barakman]

thus I do all the multiplications first and then divide at the end

Not in your original code:

return initialCap + ((initialCap * (t - DEPLOYMENT_TIME) * INFLATION) / SECONDS_PER_YEAR / 100 / 1000);

First of all, you are dividing 3 times instead of just 1 time.

Second, you are not doing it at the end of the computation (despite the fact that it appears at the end of the line).

In the line above, the last operation to take place is the addition operation, between these two intermediate values:

• initialCap
• ((initialCap * (t - DEPLOYMENT_TIME) * INFLATION) / SECONDS_PER_YEAR / 100 / 1000)

[barakman]

Maybe a good idea anyway ... ;-)

uint256 public constant INFLATION = 2000; // 2.000 %
~~~~~~~~~~~~~~~~~~~~~~~~~~
return initialCap + ((initialCap * INFLATION * (t - DEPLOYMENT_TIME)) / (SECONDS_PER_YEAR * 100 * 1000));

And as I've mentioned above, it is not clear why you divide by both 100 and 1000.

Per your example, it seems that the resolution (value of 1 unit) of INFLATION is 0.1%, or equivalently - 0.001.

So you should divide only by 1000, not by 100 * 1000.

---

As we are dealing with integer math, the 1000 is just to allow fractional percentage with 3 fractional digits , like 1234/1000 = 1.234% , while the 100 does the "percentage thing" 2% = 2/100
I was hoping if I write it that way, that it would be clear(er)

This implies misunderstanding of the concept of 'relative portion'.

A percentage is a relative portion of 1 out of 100.

A tenth (1/10) of percentage is thus a relative portion of 0.1 out of 100, or equivalently - 1 out of 1000.

You obviously need to choose the SINGLE resolution that you wish to work with, let it be 100 or 1000 or whatever.

If you choose to go along with a resolution of 1 unit representing 0.1%, then your resolution factor is 1000.

There's no need to apply "the percentage thing" (as you've called it) on top of that factor.

[SvenMeyer]

Not in your original code:

(initialCap * (t - DEPLOYMENT_TIME) * INFLATION)

that was I was referring to ... so multiplications happen here first
then we do the division (updated as you suggested)
then adding the "base" initialCap

I think the math is ok (my test confirm that)
If I want to add 2% to x
x * 1.02
x + 0.02 * x
x + 2000 / 1000 / 100 * x

[barakman]

Please read the comment above this one again (I've expanded it to address some more of your previous claims).

To put it short:

1. The last arithmetic operation in x + 2000 / 1000 / 100 is addition (+), not division (/)
2. If your INFLATION resolution is 0.1%, then you should divide by 1000, not by 100, and most certainly not by both

---

Note that:

then we do the division (updated as you suggested)
then adding the "base" initialCap

Is basically the opposite of what I've suggested - then adding the "base" initialCap means that you're NOT doing the division last, as I have suggested.