BT-235: ERC2612 - Add ERC-20 Permit function compatibility #236
Conversation
|
|
||
| pragma solidity 0.8.27; | ||
|
|
||
| import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol"; |
There was a problem hiding this comment.
Named imports are preferred when possible
There was a problem hiding this comment.
Yes, that as been done for token-factory, but in trex all import are "classical"... But ok to update it.
| EIP712Storage storage $ = _getEIP712Storage(); | ||
| // If the hashed name and version in storage are non-zero, the contract hasn't been properly initialized | ||
| // and the EIP712 domain is not reliable, as it will be missing name and version. | ||
| require($._hashedName == 0 && $._hashedVersion == 0, "EIP712: Uninitialized"); |
There was a problem hiding this comment.
Custom error instead of string could help to save some gas here
There was a problem hiding this comment.
In fact I have just copy/paste OpenZeppelin contract to be sure to not add regression (just reorder function for solhint). At first I try to just import the file, but I haven't succeed to manage several version into the package.json. I need the last version of contract-upgradeable (for the custom storage), but it have dependencies on last OpenZeppelin contracts which is not the one we are using...
So either we find a way to just import the correct version, if not we can maybe replace with a custom error.
| * @dev See {IERC-5267}. | ||
| */ | ||
| function eip712Domain() | ||
| public |
There was a problem hiding this comment.
Is there a reason why this function is public and not external? It seems to be external in the specification and I haven't seen it being explicitly called internally within the contract
There was a problem hiding this comment.
It's public in OZ implementation (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/release-v5.2/contracts/utils/cryptography/EIP712Upgradeable.sol) but yes it can be just external.
| @@ -119,7 +120,13 @@ error DefaultAllowanceAlreadyDisabled(address _user); | |||
| error DefaultAllowanceAlreadySet(address _target); | |||
|
|
|||
|
|
|||
| contract Token is IToken, AgentRoleUpgradeable, TokenStorage, IERC165 { | |||
| contract Token is IToken, AgentRoleUpgradeable, TokenStorage, IERC165, TokenPermit { | |||
There was a problem hiding this comment.
we are implementing ERC165, therefore we should add IERC20Permit to the supported interfaces
https://tokeny.atlassian.net/browse/BT-235