CEP-2612: Permit Extension#99
Conversation
davidatwhiletrue
left a comment
There was a problem hiding this comment.
@zie1ony , please check my comments.
|
|
||
| The CEP-2612 extension is defined by: | ||
| - one new entrypoint (`permit`), | ||
| - the EIP-712 typed data definition for the `Permit` struct, |
There was a problem hiding this comment.
'...and the domain separator'
|
|
||
| Below definitions use Rust syntax, but they are not Rust specific. | ||
|
|
||
| ### Entrypoint interface |
| /// disables the expiry check. | ||
| /// - `public_key` is the signer's Casper public key. | ||
| /// - `signature` is `public_key`'s signature over the EIP-712 digest | ||
| /// of the `Permit` typed data (see below). |
There was a problem hiding this comment.
I'd remove (see below) becuase this is a code block.
|
|
||
| 1. `signature` is a valid signature of `public_key` over the EIP-712 | ||
| digest of the `Permit` typed data (described below), AND | ||
| 2. `owner == Address::from(public_key)` — i.e. `owner` is the account |
There was a problem hiding this comment.
Is Address::from() correct here?
There was a problem hiding this comment.
Yes. It only represents the logic.
There was a problem hiding this comment.
I'm not convinced. Address is an Odra type. It doesn't exist in casper_types. The reader doesn't need to know that an Address is an account hash. It'd be more explicit.
| The encoded message data hashed alongside the typehash is the | ||
| concatenation, in order, of: | ||
|
|
||
| - `owner` encoded as an EIP-712 `address` (32 bytes, big-endian, left-padded), |
There was a problem hiding this comment.
In Casper, owner and spender are a hash of an Account Hash or a (package) Hash (the hash of 0x00 or 0x01 prefix || 32 bytes). I would mention it here. and remove big-endian, left-padded
There was a problem hiding this comment.
done, I mentioned how address is serialized.
| - `nonce` encoded as an EIP-712 `uint256` (32 bytes, big-endian) — equal | ||
| to the current on-chain `permit_nonces[owner]` value at the time the | ||
| signature was produced, | ||
| - `deadline` encoded as an EIP-712 `uint64` (32 bytes, big-endian, |
There was a problem hiding this comment.
“EIP-712 uint64" does not exist. u64 or uint256. let’s use the same here as in CEP-3009 for validAfter and validBefore
| The final digest is computed using the standard EIP-712 rule | ||
| (`keccak256("\x19\x01" || domainSeparator || keccak256(typeHash || encodedData))`). | ||
|
|
||
| The EIP-712 `domainSeparator` uses: |
There was a problem hiding this comment.
Not aligned with casper-eip-712 domain type:
EIP712Domain(string name,string version,string chain_name,bytes32 contract_package_hash).
Worth to add the type string to this section too.
Also, recommend to use CAIP-2 for chain_name as here.
There was a problem hiding this comment.
I updated the document, but not sure if this is what you wanted. Please check
There was a problem hiding this comment.
I don't see the domain type in the document. Because it differs from the EIP-2612 domain type, I think we need to include it.
EIP712Domain(string name,string version,string chain_name,bytes32 contract_package_hash)
Also, check this comment: https://github.com/casper-network/ceps/pull/99/changes#r3272351456
davidatwhiletrue
left a comment
There was a problem hiding this comment.
@zie1ony , see my comments.
| The final digest is computed using the standard EIP-712 rule | ||
| (`keccak256("\x19\x01" || domainSeparator || keccak256(typeHash || encodedData))`). | ||
|
|
||
| The `domainSeparator` is defined as: |
There was a problem hiding this comment.
This is not the domain separator for casper.
See here:
https://github.com/casper-ecosystem/casper-eip-712#casper-native-domain-example
EIP712Domain(string name,string version,string chain_name,bytes32 contract_package_hash)
| The final digest is computed using the standard EIP-712 rule | ||
| (`keccak256("\x19\x01" || domainSeparator || keccak256(typeHash || encodedData))`). | ||
|
|
||
| The EIP-712 `domainSeparator` uses: |
There was a problem hiding this comment.
I don't see the domain type in the document. Because it differs from the EIP-2612 domain type, I think we need to include it.
EIP712Domain(string name,string version,string chain_name,bytes32 contract_package_hash)
Also, check this comment: https://github.com/casper-network/ceps/pull/99/changes#r3272351456
|
|
||
| `Address` encoding is defined as 33 bytes, where the first byte is a type tag: | ||
| - `0x00` for `AccountHash`, | ||
| - `0x01` for `PackageHash`. |
There was a problem hiding this comment.
Hash instead of PackageHash?
o "0x01 for a package Hash"
|
@zie1ony once @davidatwhiletrue's remaining comments are addressed/responded to, we can finalize this |
| - `0x01` for `PackageHash`. | ||
|
|
||
| The encoded message data hashed alongside the typehash is the | ||
| concatenation of `CLValue` representations, in order, of: |
There was a problem hiding this comment.
I would simplify this sentence to:
"The encodeData value is the concatenation of the following EIP-712-encoded fields, in order:"
'the concatenation of CLValue representations' is not correct.
I'd use uint256 instead of U256.
Rendered