Conversation
|
|
||
| /// @notice Disables the mint() function permanently. | ||
| function freezeMinting() external auth(MINT_PERMISSION_ID) { | ||
| if (mintingFrozen) return; |
| } | ||
| } | ||
| for (uint256 i; i < _excludedAccounts.length; ) { | ||
| excludedAccounts.push(_excludedAccounts[i]); |
There was a problem hiding this comment.
We don't check for duplicates here. I do not think this causes issues but it would create redundancy.
| } | ||
|
|
||
| function delegate(address account) public override { | ||
| for (uint256 i; i < excludedAccounts.length; i++) { |
There was a problem hiding this comment.
One could use an EnumerableSet here to have O(1) lookups. This saves SLOADs on common operations like delegate/getPastVotes and transfers that gas cost to the initialization (once).
There was a problem hiding this comment.
You could also do this for account to check one cannot delegate to an excluded account
| /// @inheritdoc ERC20VotesUpgradeable | ||
| /// @dev This override extends the original implementation, ensuring that excluded addresses cannot use their voting power. | ||
| function getPastTotalSupply(uint256 timepoint) public view override returns (uint256) { | ||
| uint256 excludedSupply = super.getPastVotes(address(0), timepoint); |
There was a problem hiding this comment.
Worth considering if, given that we allow a user to specify an explicit list of excluded accounts, we should always include address(0).
I don't have a problem with this, it follows convention. But worth explaining somewhere at the very least that address 0 is always excluded.
| for (uint256 i; i < excludedAccounts.length; ++i) { | ||
| /// @dev Using getPastVotes() even though these addresses cannot self delegate. | ||
| /// @dev Another account could transfer a delegated balance to them. | ||
| excludedSupply += super.getPastVotes(excludedAccounts[i], timepoint); |
There was a problem hiding this comment.
Yeah so tradeoff here if you use enumerable set would be that 2 x O(1) SLOADs vs. the loop here.
Might have missed this but have you also factored excludedAccounts into totalSupply and not just getPastTotalSupply?
| IVotesUpgradeable private votingToken; | ||
|
|
||
| /// @notice Wether the token contract indexes past voting power by timestamp. | ||
| bool private tokenIndexedByTimestamp; |
There was a problem hiding this comment.
Might be nice to have a way to query this information with an external getter
| string memory _name, | ||
| string memory _symbol, | ||
| MintSettings memory _mintSettings | ||
| MintSettings memory _mintSettings, |
There was a problem hiding this comment.
A common use case is fixed-supply-on-mint. You may wish to consider adding freeze to the mint function but not mandatory
There was a problem hiding this comment.
That's what this PR is aiming to do
There was a problem hiding this comment.
You would need to call freeze as a separate function after deploy.
| Object.values(defaultTokenSettings), | ||
| Object.values(defaultMintSettings), | ||
| Object.values(defaultTargetConfig), | ||
| // // Provide installation inputs |
3 participants
This PR adds support for: