Replace immutable with constant for _PERMIT_TYPEHASH

[pcaversaccio]

This commit is related to the following issue discussion: OpenZeppelin/contracts-wizard#89 (comment)

Since Solidity version 0.6.12 the keccak256 of string literals is treated specially and the hash is evaluated at compile time. Since the OpenZeppelin Wizard also uses constant for OpenZeppelin's AccessControl's roles declarations, it's good practice to make this consistent.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

Hello @pcaversaccio

This indeed looks like a significant improvement. It, unfortunately, causes issues with the way our contract are transpiled to the upgradeable version.

The immutable version is currently transpiled to "normal" storage:
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol#L29

which must be initialized:
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol#L42

This is not great.

If we apply the changes you propose, we would end up with an upgradeable version that contains one less storage slot. We may be able to fix that by increasing the gap, but the upgrade plugin would consider that an error.

The bottom line is yes, we will fix that, but we need to improve the upgrade plugins in order for that fix to be transparent to users of upgradeable contracts.

[frangio]

@Amxx That should be possible to do with the same changes we are working on for #2901 regarding renaming/retyping the _initialized variable. But yes, in the meantime this is blocked by that.

[pcaversaccio]

@Amxx In light of this discussion, it could be useful to add an additional test to the GitHub actions that check the transpiled contracts for possible warnings/errors for every new PR.

[frangio]

@pcaversaccio Yes, 100%.

[frangio]

Blocked on OpenZeppelin/openzeppelin-transpiler#86.

[frangio]

This is unblocked now: a new version of the transpiler has been released with a @custom:storage-size tag that will allow us to change this to a constant variable.

[Amxx]

AFAIK, we have 2 options:

1. Add @custom:storage-size 51.
   This fill move the immutable (transpilled to private) variable inside the gap, which the upgrade plugins will see as an error.

2. Do

bytes32 private constant _PERMIT_TYPEHASH = "...."
/// @custom:oz-renamed-from _PERMIT_TYPEHASH
bytes32 private _PERMIT_TYPEHASH_DEPRECATED_SLOT;

Will will continue to reserve a slot, but not use it anymore.

[pcaversaccio]

@Amxx before we make any further adjustments wouldn't be a good idea to add a further CI transpile test? Concerning your suggestion above I personally would prefer the second option since it implements certain transparency in the code. I would go even further and add a further natspec:

bytes32 private constant _PERMIT_TYPEHASH = "...."
/**
 * @dev In previous versions `_PERMIT_TYPEHASH` was declared as `immutable`. 
 * However, to ensure consistency with the upgradable transpiler, we will continue
 * to reserve a slot.
 * @custom:oz-renamed-from _PERMIT_TYPEHASH
 */
bytes32 private _PERMIT_TYPEHASH_DEPRECATED_SLOT;

[Amxx]

Having an automated CI test to check storage compatibility is on the roadmap. Since the transpiler just received significant change, we will check everything manually, and use what we learn in the process to design the CI check

[Amxx]

bytes32 private constant _PERMIT_TYPEHASH = "...."
/**
 * @dev In previous versions `_PERMIT_TYPEHASH` was declared as `immutable`. 
 * However, to ensure consistency with the upgradable transpiler, we will continue
 * to reserve a slot.
 * @custom:oz-renamed-from _PERMIT_TYPEHASH
 */
bytes32 private _PERMIT_TYPEHASH_DEPRECATED_SLOT;

Lets get that merged.

[frangio]

It makes sense to reserve the previously used spot so 👍 to @custom:oz-renamed-from. We still need storage-size 51 though, right? Because the gap wasn't beeing calculated correctly.

[Amxx]

That is being addresses in #3252

unless you want to do it here directly

[pcaversaccio]

Probably it would make sense to put everything related to draft-ERC20Permit.sol into this PR. You let me know what you prefer and I will update accordingly.

[Amxx]

Probably it would make sense to put everything related to draft-ERC20Permit.sol into this PR. You let me know what you prefer and I will update accordingly.

the contract size will be overridden in the patches of the upgradeable repo. No need to worry about it here

[Amxx]

LGTM. @frangio any final words?

[frangio]

Looks good thank you!