Uh oh!
There was an error while loading. Please reload this page.
Migrate ERC20 contracts to Open Zeppelin 5 - #308
Conversation
| import {AccessControlEnumerable} from "openzeppelin-contracts-4/access/AccessControlEnumerable.sol"; | ||
| abstract contract MintingAccessControlOz4 is AccessControlEnumerable { |
There was a problem hiding this comment.
The new contracts/token/utils/ directory has no README.md. The repo convention (e.g. contracts/multicall/, contracts/token/erc721/), and prior review feedback on past PRs, is to add a directory README describing the contents, threat model, and audit references. Consider adding one for token/utils/.
| import {ERC20Capped} from "openzeppelin-contracts-4/token/ERC20/extensions/ERC20Capped.sol"; | ||
| import {AccessControl, IAccessControl} from "openzeppelin-contracts-4/access/AccessControl.sol"; | ||
| import {MintingAccessControl} from "../../../access/MintingAccessControl.sol"; | ||
| import {ERC20Permit, ERC20} from "openzeppelin-contracts-5/token/ERC20/extensions/ERC20Permit.sol"; |
There was a problem hiding this comment.
Now that these presets are on OZ5: the audit / threat-model status table in contracts/token/erc20/README.md (which references the earlier OZ4 commits b7adf0d7 / aa6c1d4) is stale for this contract. That README is not part of this PR -- consider updating it to record the OZ5 migration, or to note the migrated code is not yet audited against OZ5.
| @@ -66,10 +66,8 @@ contract ImmutableERC20MinterBurnerPermit is ERC20Capped, ERC20Burnable, ERC20Pe | |||
| super.renounceRole(role, account); | |||
There was a problem hiding this comment.
Non-blocking doc nit from the OZ5 migration: the NatSpec for this renounceRole override (@param account -- "The account to renounce the role from") is now slightly misleading. In OZ5, AccessControl.renounceRole renames the second parameter to callerConfirmation and enforces callerConfirmation == msg.sender (reverting with AccessControlBadConfirmation). Behaviour is unchanged vs OZ4, but consider renaming the param / updating the doc to make the self-only requirement explicit.
| function _mint(address account, uint256 amount) internal override(ERC20, ERC20Capped) { | ||
| ERC20Capped._mint(account, amount); | ||
| } | ||
| /// @inheritdoc ERC20 |
There was a problem hiding this comment.
Minor: @inheritdoc ERC20 here, while the body delegates to ERC20Capped._update. This matches OZ's own convention (OZ tags ERC20Capped._update with @inheritdoc ERC20), so it is acceptable, but @inheritdoc ERC20Capped would more precisely describe the delegation target. Non-blocking.
Migrate ERC 20 contracts to use Open Zeppelin.
Move MintingAccessControl contracts from contracts/access to contracts/token/utils