Permit [eip-2612] support for AMPL #181
Conversation
aalavandhan
force-pushed
the
permit
branch
3 times, most recently
from
e37ea91 to
ccfd317
Compare
|
Will wait for @ahnaguib to review this |
ahnaguib
changed the title
contracts/UFragments.sol
Outdated
| bytes32 permitDataDigest = | ||
| keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, ownerNonce, deadline)); | ||
| bytes32 digest = | ||
| keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, permitDataDigest)); |
There was a problem hiding this comment.
@ahnaguib How OpenZeppelin implements this. It is identical
_TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
_PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
domainSeperator = keccak256(
abi.encode(
_TYPE_HASH,
name,
version,
_getChainId(),
address(this)
)
);
structHash = keccak256(
abi.encode(
_PERMIT_TYPEHASH,
owner,
spender,
amount,
_nonces[owner].current(),
deadline
)
);
digest = keccak256(abi.encodePacked("\x19\x01", domainSeperator, structHash));
|
Holding off on this until we bump up the token solidity compiler version |
|
OZ released Permit contract today: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/drafts/ERC20Permit.sol |
|
@brandoniles I've updated to the new implementation which queries the chainId everytime |
contracts/UFragments.sol
Outdated
| /** | ||
| * @return The underlying blockchain's chainID. | ||
| */ | ||
| function getChainId() public view returns (uint256) { |
There was a problem hiding this comment.
Does this need to be public?
Also, basic question but--does this being its own function use more gas than it would if it were just in the body of DOMAIN_SEPARATOR()?
There was a problem hiding this comment.
Makes sense. Since it's not being used anywhere else doesn't have to be a public function
Gasless approvals with permit.
https://eips.ethereum.org/EIPS/eip-2612
Reference implementations
There's one major discussion point.
As per the standard openzeppelin implementation, the domain separator is computed in the contract's constructor. However since we have a live contract instance, we cant do that.
In the current approach, I've hardcoded the values that go into the domain separator creation based on the production config. (Address, chainId and token name).
If we dont want to hardcode this, we could create a function which we invoke after upgrade which sets the domain separator after reading the contract's store (more general solution).