Governor shadows EIP-712's `_name`

[ernestognw]

📝 Details

Governor initializes with the following constructor:

constructor(string memory name_) EIP712(name_, version()) {
    _name = name_;
}

However, _name is also the name of the variable in EIP712:

constructor(string memory name, string memory version) {
    _name = name.toShortStringWithFallback(_nameFallback);
    _version = version.toShortStringWithFallback(_versionFallback);
    _hashedName = keccak256(bytes(name));
    _hashedVersion = keccak256(bytes(version));

    _cachedChainId = block.chainid;
    _cachedDomainSeparator = _buildDomainSeparator();
    _cachedThis = address(this);
}

Apparently, there's no point in having the storage variable in the Governor when the name in EIP712 can be used.

[frangio]

There is a point about shadowing, which IMO is not concerning, and a point about redundancy. A similar thing comes up in ERC20 with ERC20Permit, since it needs a name for EIP712 and ERC20 already has one.

[balajipachai]

There is a point about shadowing, which IMO is not concerning, and a point about redundancy. A similar thing comes up in ERC20 with ERC20Permit, since it needs a name for EIP712 and ERC20 already has one.

Talking about redundancy can't we just remove _name from Governor contract?

[balajipachai]

@frangio @Amxx Any updates on this?

[RenanSouza2]

Is the governor's name suposed to be able to be overriden by other contracts? Because if not I suggest to just expose the _EIP712Name in the name function